Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

API docs and the live API disagree #820

Closed
algernon opened this issue Nov 6, 2023 · 9 comments
Closed

API docs and the live API disagree #820

algernon opened this issue Nov 6, 2023 · 9 comments

Comments

@algernon
Copy link

algernon commented Nov 6, 2023

Overview

I'm writing an API client in Rust, and have been following the API docs for the most part. Unfortunately, I ran into a couple of cases where the docs are inaccurate, and other cases where the API is just... strange or inconsistent.

I hope you don't mind that I'm reporting multiple problems in one issue: they are all somewhat related, anyway.

Application configuration

  • Single mode or Multi-user mode?: multi-user, but doesn't matter for these issues.
  • Database? sqlite. This might be relevant, I have not tried with MySQL (and have no intention to, sorry)
  • Open registration? Nope, but shouldn't be relevant.
  • Federation enabled? Nope, but shouldn't be relevant.

Version or last commit: 0.14.0

API issues

Updating a Collection's visibility

The docs say that if I want to update a Collection's visibility - to, say, public - I need to send {"visibility": 1} to the /api/collections/{alias} endpoint in a POST request.

However, that doesn't work.

Steps to reproduce

❯ curl -s "http://localhost:9080/api/collections/algernon" \
       -H "content-type: application/json" \
       -H "Authorization: Token ${TOKEN}" \
       -X POST -d '{"visibility":1}'
{"code":400,"error_msg":"Supply some properties to update."}

Workaround

Digging around in the source, I discovered that I need to use the public property for this. That works.

Expected behaviour

However, when retrieving a collection's info, public is serialized as a bool, so I have no way of knowing whether the blog is unlisted, private, or password protected. The only state I can be sure of is if public: true, then the collection is public. If it is false, it can be either of the three.

It would be nice if the API matched the documentation, and that if visibility would be included in the collection properties in response to a GET request to /api/collections/{alias}. Keeping public there too, for compatibility is fine too. I just wish I could both retrieve the full range of visibility settings, and set them consistently: with the same key, and same type as they are retrieved.

Some optional string properties (of a Collection) are inconsistently de/serialized

I first noticed the problem when trying to set a custom style sheet for a collection via the /api/collections/{alias} update API, and the API telling me that it expects valid JSON.

Steps to reproduce

❯ curl -s "http://localhost:9080/api/collections/algernon" \
           -H "content-type: application/json" \
           -H "Authorization: Token ${TOKEN}" \
           -X POST -d '{"style_sheet": "/* comment */"}'
{"code":400,"error_msg":"Expected valid JSON object."}

I would have expected this to work, because if I set the style via the web UI, retrieving the collection info has it serialized as a string:

❯ curl -s "http://localhost:9080/api/collections/algernon" \
           -H "content-type: application/json" \
           -H "Authorization: Token ${TOKEN}" | jq .data.style_sheet
"/* comment */"

Workaround

A bit of searching around, I found that Go's sql.NullString is picky, and in order for Go to correctly deserialize the JSON string value to sql.NullString, I need to use a different format:

{
  "style_sheet": {
    "String": "/* comment */",
    "Valid": true
  }
}

Using that as the payload for the collection update request does the right thing.

Expected behaviour

I'd expect that the format I receive when retrieving the collection info, can be fed back to the update directly. That it'd use the same format. I don't mind either the string format, or the more verbose NullString thing, but it would be fantastic if the API was consistent.

Or if the docs documented both formats.

Also applies to

This also applies to script and signature too.

Some optional properties (of a Collection) are less optional than others

Some optional properties of a Collection, such as verification_link, description, and style_sheet seem to be included in the /api/collections/{alias} GET response, even if they're empty: they appear as "":

❯ curl -s "http://localhost:9080/api/collections/algernon" \
           -H "content-type: application/json" \
           -H "Authorization: Token ${TOKEN}" | jq .data.description
""

...while others, such as script, and signature do not appear in this, unless they're set. This makes it a tiny bit harder to implement serialization and deserialization for the Collection type, because I have to treat some of the optional properties more specially than the others.

Closing thoughts

While I found workarounds for all of the issues above, it would make it much easier to use the API, if the problems listed above were fixed. There may be other issues and inconsistencies too, that I have not discovered yet - I barely started exploring the API. If I find further issues, I will report them. Either as a followup to this issue here, or a separate one, whichever you find more convenient.

thebaer added a commit that referenced this issue Nov 7, 2023
Use standard string instead of sql.NullString for `style_sheet`, `script`, and `signature`.

Addresses #820
@algernon
Copy link
Author

algernon commented Nov 8, 2023

Another thing I discovered today: the created and updated properties of posts appear as RFC3339 formatted strings in API responses, but they must appear as %F %T (ie, %Y-%m-%d %H:%M:%S, without any timezone specifier) when sending them back to the API.

I think both responses & inputs should use the same format (probably RFC3339, which is what the documentation says).

@algernon
Copy link
Author

algernon commented Nov 8, 2023

Another one, a minor inconsistency: the same thing that appears as appearance in API responses, must be font in data payloads. The documentation is correct in this case, but this feels like an inconsistency nevertheless.

@algernon
Copy link
Author

algernon commented Nov 9, 2023

Another thing I discovered today: the created and updated properties of posts appear as RFC3339 formatted strings in API responses, but they must appear as %F %T (ie, %Y-%m-%d %H:%M:%S, without any timezone specifier) when sending them back to the API.

I think both responses & inputs should use the same format (probably RFC3339, which is what the documentation says).

As a followup to this, I discovered another inconsistency with dates, this one is considerably trickier to workaround: when I create a new post (either via /api/collections/{alias}/posts, or /api/posts), the created property (and likely updated too, I haven't verified that yet) is ignored unless it is in RFC3339 format. Other parts of the API expect the format in %F %T, and error out if the format isn't that. These two endpoints ignore the property if it fails to deserialize.

Reproduction:

❯ curl -s "http://localhost:9081/api/posts" \
    -H "content-type: application/json" \
    -H "Authorization: Token ${TOKEN}" \
    -X POST -d '{"body":"test", "created":"2023-11-10 12:00:00"}' \
    | jq -r .data.created
2023-11-09T00:07:24Z

Notice that the date I set is different from the one returned after post creation. Changing the date format results in the correct created date:

❯ curl -s "http://localhost:9081/api/posts" \
    -H "content-type: application/json" \
    -H "Authorization: Token ${TOKEN}" \
    -X POST -d '{"body":"test", "created":"2023-11-10T12:00:00Z"}' \
    | jq -r .data.created
2023-11-10T12:00:00Z

However, trying to update a post with RFC3339 format date fails:

❯ curl -s "http://localhost:9081/api/posts/82rxw75xdc" \
    -H "content-type: application/json" \
    -H "Authorization: Token ${TOKEN}" \
    -X POST -d '{"body":"test", "created":"2023-11-10T12:00:00Z"}'
{"code":500,"error_msg":"This is an unhelpful error message for a miscellaneous internal error."}

In the logs, I see:

ERROR: 2023/11/09 01:14:43 database.go:792: Unable to parse Created date: parsing time "2023-11-10T12:00:00Z" as "2006-01-02 15:04:05": cannot parse "T12:00:00Z" as " "

I can work this around in my client library by sending an update request immediately after a create, if the request had a created or updated property. But that's... not very nice. Serializing one way for one endpoint, and another way for the other would be even uglier.

@algernon
Copy link
Author

algernon commented Nov 9, 2023

While writing some tests for my library, stumbled upon another minor inconsistency: trying to retrieve a non-existent post will always return a response in text/plain, whether authenticated or not.

Reproduction:

❯ curl -i http://localhost:8080/api/posts/0 -H "content-type: application/json"
HTTP/1.1 404 Not Found
Cache-Control: public, max-age=604800, immutable
Content-Type: text/plain; charset=utf-8
X-Content-Type-Options: nosniff
Date: Thu, 09 Nov 2023 18:19:02 GMT
Content-Length: 19

404 page not found

Update: same thing happens when I try to post an update, or delete a non-existing post: the response is in text/plain. Looks like the entire /api/posts/{id} endpoint returns text/plain when the id doesn't exist, rather than the usual json.

@algernon
Copy link
Author

algernon commented Nov 9, 2023

One more: contrary to the documentation: creating a collection requires the title property:

❯ curl -s 'http://localhost:8080/api/collections' \
       -H "content-type: application/json" \
       -H "Authorization: Token ${TOKEN}" \
       -H "accept: application/json" \
       -X POST -d '{"alias":"thing"}'
{"code":400,"error_msg":"Parameter(s) `title` required."}

If I change the payload to {"title":"thing"}, that works.

@thebaer
Copy link
Member

thebaer commented Feb 2, 2024

Thanks for reporting all of this! I've fixed one issue here as noted below, and that will go out in the next release. Otherwise, I'll address other issues here, and would appreciate if you could open separate issues where indicated just to help us make sure each one gets tackled.

  • Updating a Collection's visibility: agreed, this definitely needs a fix / to support that use case. Could you please move this to a separate issue?
  • Some optional string properties (of a Collection) are inconsistently de/serialized: this is fixed in Fix Collection property serialization on API #854.
  • Some optional properties (of a Collection) are less optional than others: agreed this also needs a fix -- could you move it to a separate issue?
  • Date inconsistencies: can you open this as a separate issue?
  • text/plain responses: can you open this as a separate issue?
  • Required title for collection creation: no action needed -- I'll update this in the docs, thanks!

@algernon
Copy link
Author

algernon commented Feb 2, 2024

Yep, I can open separate issues! Will do so later today.

@thebaer
Copy link
Member

thebaer commented Feb 2, 2024

Thank you! Awesome to see a Rust library come together, by the way. Will be happy to promote it on our developer site.

Once those issues are created, we can consider this issue closed by #854.

@algernon
Copy link
Author

algernon commented Feb 2, 2024

#864, #865, #866, #867 opened for the issues that were remaining, so this one can be closed, indeed.

@algernon algernon closed this as completed Feb 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants