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

adapt test for single user using hashtag as id #211

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

TieyanFu
Copy link

@TieyanFu TieyanFu commented May 16, 2023

In rfc below case does not necessarily require 404 when trying to get user with id #. In this case, the hashtag as user ID could also be considered bad request 400.

Could you please check if this could be included in existing test case? Certainly the name of the test case has been changed because it is actually getting single user instead of all users, i.e.: resourceAwareUserRequest.readSingleUser("#") ). We could also keep the original one and add a different expecting status code 400, so that other stakeholder would not have a breaking change.

@cla-assistant
Copy link

cla-assistant bot commented May 16, 2023

CLA assistant check
All committers have signed the CLA.

@karaulyanovsap
Copy link
Member

I agree that the "#" is not a good choice for a globally unique user ID but it is up to the service provider to issue and ensure that the user IDs are unique. If the service provider handles GET /Users/# as 400 Bad Request (imagine that the # is url encoded here as %23) then GET /Users/?filter=id eq "#" should also be considered "400 Bad Request" although the filter has a completely valid syntax. According to https://www.rfc-editor.org/rfc/rfc7644#section-3.12:

| 400 (Bad       | GET, POST,     | Request is unparsable,           |
| Request)       | PUT, PATCH,    | syntactically incorrect, or      |
|                | DELETE         | violates schema.                 |

GET /Users/%23 fits none of the above and should be in general a valid request even if there are no users with id=#.

@whitelake-city
Copy link

whitelake-city commented May 22, 2023

@karaulyanovsap thanks for the reply.

I am not sure I can agree with your statement:

If the service provider handles GET /Users/# as 400 Bad Request (imagine that the # is url encoded here as %23) then GET /Users/?filter=id eq "#" should also be considered "400 Bad Request" although the filter has a completely valid syntax

The service provider defines the schema for the id. Our DBMS only allows querying users by uuid. In case something else is passed, the query will fail.

In order to make this "work", we would need to treat any id passed to any endpoint differently to the rest of the data, by doing a pre-check if it is valid and then returning a 404 in case it is not a uuid. Other violations, would of course return a 400 bad request.

To think this further, what would you do about a string that contains html or javascript? This would comply to the schema of a string. Applying the same logic, is that then also a 404???

As you already mentioned: "#" is not a good choice and I think it would be easy to write the test in a way so that uniqueness is tested. If you cannot agree what being said, that's also ok. However we will then kindly ignore this test from our side.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants