404 not implemented on PUT /api/tags/{unknown}

[2ynn]

PUT /api/tags/{unknown} does indeed return a 400 response when no data is passed, which is tested in test_valid_id
However, when passing valid data to the request, the response is 200 even when the tag doesn't exist. It should be 404 instead.
To reproduce, add these lines to test_tag_api and it should pass:

    rd = client.put('/api/tags/unknown', data={'tags': 'tag3,tag4'})
    assert rd.status_code == 200
    assert rd.get_json() == response_template['success']

In curl:

curl -X 'PUT' 'http://localhost:5000/api/tags/unknown' -H 'accept: application/json' -H 'Content-Type: application/json' -d '{                                       
  "tags": "tag3,tag4"
}'                                                                                               
{
  "message": "success",
  "status": 0
}

I'd be happy to work on this. The get method in ApiTagView already raises the 404 so i would rely on it.

[LeXofLeviafan]

…Is the problem that you're expecting the PUT method to return 404 if the tag doesn't exist?

The PUT method of /api/tags/ is equivalent to the --replace CLI argument; so I don't see much of an issue if it doesn't respond with an error when trying to replace a nonexistent tag (since that matches library behaviour). Although @jarun or @rachmadaniHaryono might have a different opinion on this.

There are, however, some issues I've noticed while researching this one:

• replace_tag() can't be called with an empty tags list (i.e. for tag deletion) when using buku as a library (because delete_tag_at_index() prompts for user input unless an argument is specified – unlike all other places where self.chatty is used instead – and replace_tag() doesn't specify this argument when using delete_tag_at_index()).
  Suggested solution: change delete_tag_at_index() to use self.chatty, or add a keyword argument to replace_tag() (i.e. confirm_delete=True)
• The empty value ({"tags": ""}) is equivalent to deleting the tag, and it returns 400 most likely because of the previous issue; because of that, tag deletion isn't supported by bukuserver API.
  Suggested solution: add a DELETE method to the API (e.g. using delete_tag_at_index() directly).
• When trying to pass multiple tags ({"tags": "foo,bar"}), the result is surprisingly a single tag with commas replaced with spaces (the value is split into substrings with comma as separator, then replace_tag() passes it to parse_tags(), and the latter treats them as CLI/shell parameters).
  Suggested solution: remove token parsing functionality from replace_tag(), and accept either a list of tags or a string joined by comma instead (it's only used once in the library itself, and the parsing can be done when passing the argument)
• Also, though not directly related, there seem to be issues with --db and --nostdin CLI arguments (which I found out when comparing CLI and API functionality on a temporary DB file).

[LeXofLeviafan]

Also this:

• GET on /api/tags responds with a list of tags, but the same URI with added slash (/api/tags/, which would normally be considered the same) returns HTTP 404.
  Suggested solution: add strict_slashes=False to the endpoint definition

[2ynn]

Is the problem that you're expecting the PUT method to return 404 if the tag doesn't exist?

Yes exactly

I don't see much of an issue if it doesn't respond with an error when trying to replace a nonexistent tag (since that matches library behaviour).

Ok I had not realized it was the expected behaviour (I have not spent much time exploring the CLI). From an API perspective I find it disturbing to receive a success message when an action was not actually performed. But I guess we could argue it was performed on 0 objects...

[2ynn]

• When trying to pass multiple tags ({"tags": "foo,bar"}), the result is surprisingly a single tag with commas replaced with spaces

This behavior also surprised me a lot but it seemed to be the expected one as is shown in the test_tag_api:

rd = client.put('/api/tags/tag1', data={'tags': 'tag3,tag4'})
    assert rd.status_code == 200
    assert rd.get_json() == response_template['success']
    rd = client.get('/api/tags')
    assert rd.status_code == 200
    assert rd.get_json() == {'tags': ['tag2', 'tag3 tag4']}

[LeXofLeviafan]

it seemed to be the expected one as is shown in the test_tag_api

Problem is, it's a) unintuitive, and b) doesn't match CLI behaviour (--replace foo bar,baz replaces tag foo with tags bar & baz).

[2ynn]

• remove token parsing functionality from replace_tag(), and accept either a list of tags or a string joined by comma instead (it's only used once in the library itself, and the parsing can be done when passing the argument)

I agree it would be cleaner to accept a list of strings instead of a single comma-separated string on and the use of parse_tags() does seem awkward in the case of the API. However, because this function does some cleaning on the keywords (lowercasing, removing duplicates) and it is also used in ApiTagView.update_model() i am not sure i feel comfortable refactoring. As an alternative I would suggest wrapping the comma-separated string into a list (see #676).

[LeXofLeviafan]

lowercasing, removing duplicates

You mean like set(s.lower() for s in new_tags)?

[LeXofLeviafan]

does seem awkward

I think that “if you want to pass the list ['foo bar', 'baz'] then the actual value you need to pass is ['foo', 'bar,baz']” goes beyond ‘awkward’ and straight into the ‘terrible’ territory 😅

[LeXofLeviafan]

If you want to minimize logic changes, it could be changed the other way around: making the function accept a string (joined with spaces in case of CLI), and then the space/comma behaviour won't get in the way of comprehending the API contract. The "wrap in a single-item list" workaround can be done within the function then.

[2ynn]

Look i totally agree that 'awkward' is an understatement. All I have done was following what was done here.

I am completely new to this project and was hoping to build off the API (which I started documenting manually using swagger), not rewriting inner workings that could potentially affect other features, especially since I haven't played with the CLI. I was also very confused from the get go by the way the tests were written (e.g., which brought me sideways.

Thank you for taking the time to clarify all this with me and pointing out what needed to be changed. I think I get a better sense now that the API might not yet do exactly what it's supposed to do and it's not just documentation that is lacking.