Inconsistencies between implementation and API spec #83

patriciagarcia · 2016-02-18T13:31:55Z

It seems that the current implementation is not completely compatible with the API spec, here is the list of differences I found, for creating follow up issues.

Some things to take into account:

to look for inconsistencies, I only checked the tests, not the actual implementation, I thought it was a better idea because this way I can find all the missing tests too.
sometimes the bug is in the API spec, sometimes in the code, when I say "it should do x instead of y", I mean that it should be that according to the spec. When the problem is in the spec, I mention it explicitly.
some of the comments are not even bugs but something I am not sure it's right, in that case it's mentioned as "Question".
some of them are repeated in many calls and they might be better solved by creating a test helper that can be reused (for example with the 409 and 401 and 400 errors).
please, don't dismiss them lightly assuming that is the API what is wrong (instead of the implementation), the API spec took a lot of work and discussion, everything that is in there has been thought carefully. That doesn't mean there is no mistakes, of course! but it means that if something it's going to be changed it should be considered carefully too ;-). Also, it should be considered if it affects other parts of the spec.

Let me know if there are any questions!

Session

PUT /session

PUT /session to return 401 Invalid credentials, not 401 Invalid password #85 in the test 'User found' + 'Invalid password', the error message to return should be Invalid credentials instead of Invalid password
PUT /session to return 401 Invalid credentials, not 401 Invalid password #85 same for the the test 'User is admin' + 'Invalid password' (Invalid password -> Invalid credentials)
return helpful error message for invalid data.type in payload #90 in the test 'type is not provided' (inside utils/invalid-type-errors.js) the error message to return should be data.type must be 'session' instead of child "data" fails because [child "type" fails because ["type" is required]]
return helpful error message for invalid data.type in payload #90 in the test 'type is not supported' (inside utils/invalid-type-errors.js) the error message to return should be data.type must be 'session' instead of child "data" fails because [child "type" fails because ["type" must be one of [session]]]
Error message if "authorization" Header is not allowed #92 in the test 'Authorization header provided' (in utils/authorization-header-not-allowed-error.js) the error message to return should be Authorization header not allowed instead of child "authorization" fails because ["authorization" is not allowed]
PUT /session to return 400 Bad Request for invalid ?include=<path> #87 according to JSON API spec: the query string ?include=<path> with any unsupported path should return 400 Bad Request, we currently return 403 Forbidden

PUT /session?include=account.profile

PUT /session?include=account.profile -> 400 Bad request for admins #93 If user is admin, respond with 400 Bad Request as per JSON API spec

GET /session

When authorization is required but missing, return with 401 #94 in the test 'No Authorization header sent' the error to return should be 401, "Authorization header missing" instead of 403
GET /session: 404 should be 401 #95 in the test 'User not found' the error to return should be 401, "Session invalid" instead of 404
GET /session: 404 should be 401 #95 in the test 'User found' + 'Session invalidthe error to return should be401, "Session invalid"instead of404`
PUT /session to return 400 Bad Request for invalid ?include=<path> #87 There is a test with GET /session?include=foobar, according to JSON API spec this should return 400 Bad Request instead of 403

DELETE /session

When authorization is required but missing, return with 401 #94 in the test 'No Authorization header sent' the error to return should be 401, "Authorization header missing" instead of 403
DELETE /session: 404 should be 401 #96 in the test 'User not found' the error to return should be 401, "Session invalid" instead of 404
DELETE /session: 404 should be 401 #96 in the test 'User found' + 'Session invalid the error to return should be 401, "Session invalid" instead of 404
add a test for /DELETE /session?include=foobar and test that it returns 400 Bad Request according to JSON API spec

Account

PUT /session/account

PUT /session/account 409 error message #97 In the test 'CouchDB User already exist' the error returned is 409 Document update conflict, but should be 409: An account with that username already exists
return helpful error message for invalid data.type in payload #90 in the test 'type is not provided' the error message to return should be data.type must be 'account' instead of child "data" fails because [child "type" fails because ["type" is required]]
return helpful error message for invalid data.type in payload #90 in the test 'type is not supported' the error message to return should be data.type must be 'account'', instead of child "data" fails because [child "type" fails because ["type" must be one of [account]]]
Error message if "authorization" Header is not allowed #92 in the test 'Authorization header provided' (in utils/authorization-header-not-allowed-error.js) the error message to return should be Authorization header not allowed instead of child "authorization" fails because ["authorization" is not allowed]
add a test PUT /session/account?include=foobar and test that server responds with 400 Bad Request as defined in JSON API spec

GET /session/account

When authorization is required but missing, return with 401 #94 missing test: should return 401, "Authorization header missing" if the request misses the authorization header
in the test 'Session does not exist' the error to return should be 401, "Session invalid" instead of 404
GET /session/account: "404 Admins have no accounts" error #98 If user is admin, return 404 "Admins have no accounts" instead of 403
In GET /session/account?include=foobar test, server response should be `400 Bad Request according to JSON API spec

PATCH /session/account

When authorization is required but missing, return with 401 #94 in test 'No Authorization header sent' the error to return should be 401, "Authorization header missing" instead of 403
PATCH /session/account -> 204 #99 in the test 'changing password' the response should return 204 with empty body (!) instead of 201
missing test: should return 401, "Session invalid" if the session is not valid
409 "data.id must be '{id}'" error
add test for PATCH /session/account?include=foobar and test that server returns 400 bad request as per JSON API spec

DELETE /session/account

in the test 'without valid session' the error to return should be 401, "Session invalid" instead of 404
When authorization is required but missing, return with 401 #94 missing test: should return 401, "Authorization header missing" if the request misses the authorization header
add test for DELETE /session/account?include=foobar and test that server returns 400 bad request as per JSON API spec

Profile

GET /session/account/profile

When authorization is required but missing, return with 401 #94 missing test: should return 401, "Authorization header missing" if the request misses the authorization header
GET /session/account/profile: "404 Admins have no profiles" error #101 If user is admin, return 404 Admins have no accounts instead of 403
add test for GET /session/account/profile?include=foobar and test that server returns 400 bad request as per JSON API spec

PATCH /session/account/profile

PATCH /session/account/profile -> 204 #102 in the test 'Session does exist' the response should return 204 with empty body (!) instead of 201
When authorization is required but missing, return with 401 #94 missing test: should return 401, "Authorization header missing" if the request misses the authorization header
PATCH /session/account/profile: "404 Admins have no profiles" error #103 If user is admin, return 404 Admins have no profiles instead of 403
PATCH /session/account/profile -> 409 "data.id must be '{id}'" error #104 409 "data.id must be '{id}'" error
add test for PATCH /session/account/profile?include=foobar and test that server returns 400 bad request as per JSON API spec

Requests

In all relevant /requests routes

add test for <METHOD> /requests?include=foobar and test that server returns 400 bad request as per JSON API spec

POST /requests

return helpful error message for invalid data.type in payload #90 in the test 'type is not provided' the error message to return should be data.type must be 'request' instead of child "data" fails because [child "type" fails because ["type" is required]]
return helpful error message for invalid data.type in payload #90 in the test 'type is not supported' the error message to return should be data.type must be 'request' instead of child "data" fails because [child "type" fails because ["type" is required]]

Admins

User account collection

POST /accounts

When authorization is required but missing, return with 401 #94 in the test 'No Authorization header sent' the error to return should be 401, "Authorization header missing" instead of 403
POST /accounts Add test for 401, "Session invalid" if user is not an admin #124 Add test for 401, "Session invalid" if user is not an admin
POST /accounts Add test for 401 because of invalid session #125 Add test for 401, "Session invalid" when the session is invalid
return helpful error message for invalid data.type in payload #90 in the test 'type is not provided' the error message to return should be data.type must be 'account' instead of child "data" fails because [child "type" fails because ["type" is required]]
return helpful error message for invalid data.type in payload #90 in the test 'type is not supported' the error message to return should be data.type must be 'account'', instead of child "data" fails because [child "type" fails because ["type" must be one of [account]]]
add a test POST /accounts?include=foobar and test that server responds with 400 Bad Request as defined in JSON API spec

GET /accounts

When authorization is required but missing, return with 401 #94 in the test 'No Authorization header sent' the error to return should be 401, "Authorization header missing" instead of 403
GET /accounts Add test for 401, "Session invalid" if user is not an admin #136 Add test for 401, "Session invalid" if user is not an admin
[hoodie-account-server] GET /accounts Add test for 401 "Session invalid" camp#62 Add test for 401, "Session invalid" when the session is invalid
[hoodie-account-server] Add test for GET /accounts?include=foobar camp#63 add a test GET /accounts?include=foobar and test that server responds with 400 Bad Request as defined in JSON API spec

GET /accounts/id

Error missing from API spec: 404 when account not found
When authorization is required but missing, return with 401 #94 Add test for 401, "Authorization header missing"
[hoodie-account-server] Add test for GET /accounts/{id} with invalid session camp#64 Add test for 401, "Session invalid" if user is not an admin
[hoodie-account-server] Add test for GET /accounts/{id} if user is not an admin camp#65 Add test for 401, "Session invalid" when the session is invalid
[hoodie-account-server] Add test for GET /accounts/123?include=foobar camp#66 add a test GET /accounts/123?include=foobar and test that server responds with 400 Bad Request as defined in JSON API spec

PATCH /accounts/id

When authorization is required but missing, return with 401 #94 in the test 'No Authorization header sent' the error to return should be 401, "Authorization header missing" instead of 403
[hoodie-account-server] Add test for PATCH /accounts/{id} with invalid session camp#67 missing test for 401, "Session invalid"
[hoodie-account-server] Add test for PATCH /accounts/{id} if user is not an admin camp#68 Add test for 401, "Session invalid" if user is not an admin
[hoodie-account-server] PATCH /accounts/{id} should respond with 204 unless ?include=profile query set camp#70 success should return 204 with empty body (!) instead of 201
return helpful error message for invalid data.type in payload #90 Add test: if data.type is not account, server must respond with data.type must be 'account'
[hoodie-account-server] Add test for PATCH /accounts/{id} if account not found camp#69 Add test for 404 when account not found
[hoodie-account-server] Add test for PATCH /accounts/123 with invalid type / id attributes camp#73 missing test: should return 409, "'type' and 'id' provided don't match any existing document" if type and/or id in the request are not correct or missing. Also remove call to invalid-type-error.js (!)
[hoodie-account-server] Add test for PATCH /accounts/123?include=foobar camp#71 add a test PATCH /accounts/123?include=foobar and test that server responds with 400 Bad Request as defined in JSON API spec

DELETE /accounts/id

When authorization is required but missing, return with 401 #94 in the test 'No Authorization header sent' the error to return should be 401, "Authorization header missing" instead of 403
[hoodie-account-server] Add test for DELETE /accounts/{id} with invalid session camp#74 missing test for 401, "Session invalid" when the session is invalid (there is a pending test 'CouchDB session invalid')
[hoodie-account-server] Add test for DELETE /accounts/{id} with user session camp#75 Add test for 401, "Session invalid" if user is not an admin
[hoodie-account-server] Add test for DELETE /accounts/{id} if account not found camp#76 Add test for 404 when account not found
[hoodie-account-server] Add test for DELETE /accounts/123?include=foobar camp#78 add a test DELETE /accounts/123?include=foobar and test that server responds with 400 Bad Request as defined in JSON API spec

The text was updated successfully, but these errors were encountered:

passcod · 2016-02-18T19:37:36Z

Also, unless I've missed it, the spec doesn't mention that admins don't have a profile, as this issue suggests.

gr2m · 2016-02-18T19:48:28Z

the handling of admins is specific to Hoodie, it’s out of scope of the generic account JSON API spec

passcod · 2016-02-18T20:57:11Z

That's fine, but the spec has this line:

Every account has an associated profile, by default, an empty one is created when creating the account.

Which to me parses as:

Every account has an associated profile.
By default, an empty one is created when creating the account.

So are Hoodie admins non-compliant or should the spec mention that profiles are actually optional? Or alternatively, should this line in the spec be parsed as:

Every account has an associated profile by default.
An empty one is created when creating the account.

gr2m · 2016-02-18T21:07:13Z

yes, I’m sorry, Hoodie admins are not actual accounts that you can sign in with, they are only used to manage your app settings, other accounts, etc.

So we implement the Spec for normal user accounts in hoodie-server-account, Admins are treated differently, but that is special to Hoodie and does not need to be specified in the Spec. The Spec is not tied to Hoodie

breun · 2016-02-21T22:56:52Z

The second item under PUT /session says Invalid credentials -> Invalid password, but it should be the other way around. :o)

gr2m · 2016-02-21T22:58:54Z

fixed, thanks @breun

gr2m · 2016-02-24T05:40:50Z

according to JSON API spec: the query string ?include= with any unsupported path should return 400 Bad Request. Add this error to the spec and add a test and the implementation for it to the code.

I suggest we simply reference the JSON API query parameters instead of repeating the JSON API spec in our own. I’ve started a PR here: hoodiehq/account-json-api#23 any thoughts?

gr2m · 2016-02-25T21:52:12Z

I think all issues regarding the spec not mentioning query arguments like PUT /session?include=account.profile have been addressed by hoodiehq/account-json-api#23 – @patriciagarcia would love your feedback on this

gr2m · 2016-02-28T00:07:38Z

@patriciagarcia I’m going and rewording your list to make it simpler to create starter issues out of them. I removed this these

POST /requests

missing error to add to the spec: for requests that don't support authentication, it returns 403 if an authorization header is provided.

I think if authentication is not needed, it can be simply ignored, no need to throw an error? Maybe there is a test case for that though :)

GET /accounts

Question: what error should be returned when the user is not an admin? I'd say 401. This needs to be tested and added to the API spec.

we already have the 401 Session invalid in the spec. I’ve reworded it to make sure we have a test for that

GET /accounts

according to JSON API spec: the query string ?include=<path> with any unsupported path should return 404 Bad Request. Add this error to the spec and add a test and the implementation for it to the code.

hoodiehq/hoodie-account-server#83

resolves the errors mentiond in #83 * * * This commit was sponsored by Neighbourhoodie You can hire Neighbourhoodie for all your Hoodie / CouchDB / Offline First needs http://go.hood.ie/thehoodiefirm

gr2m · 2016-10-15T23:28:27Z

@patriciagarcia hey Pat, I know it’s been a looooooong while, but can you recall why you suggested to "Also remove call to invalid-type-error.js (!)" for PATCH /accounts/123?

gr2m · 2016-10-16T00:50:40Z

yay finally created issues for all the things that Patricia found :) Took only 8 months but hey ✌️ Thanks again for your great help Patricia 👏

gr2m added the in progress label Feb 20, 2016

gr2m self-assigned this Feb 20, 2016

gr2m mentioned this issue Feb 21, 2016

PUT /session to return 401 Invalid credentials, not 401 Invalid password #85

Closed

8 tasks

gr2m mentioned this issue Feb 24, 2016

Query paramters hoodiehq/account-json-api#23

Merged

gr2m mentioned this issue Feb 24, 2016

PUT /session to return 400 Bad Request for invalid ?include=<path> #87

Closed

8 tasks

gr2m mentioned this issue Feb 27, 2016

Fix all known issues for Hoodie Camp Release gr2m/milestones#88

Closed

32 tasks

gr2m added a commit to hoodiehq/account-json-api that referenced this issue Feb 28, 2016

docs(api blueprint): fix inconsistencies based on @patriciagarcia review

fba3e73

hoodiehq/hoodie-account-server#83

gr2m mentioned this issue Feb 28, 2016

return helpful error message for invalid data.type in payload #90

Closed

gr2m added a commit that referenced this issue Feb 28, 2016

test: validate payload

225328b

resolves the errors mentiond in #83 * * * This commit was sponsored by Neighbourhoodie You can hire Neighbourhoodie for all your Hoodie / CouchDB / Offline First needs http://go.hood.ie/thehoodiefirm

This was referenced Feb 28, 2016

Error message if "authorization" Header is not allowed #92

Closed

PUT /session?include=account.profile -> 400 Bad request for admins #93

Closed

When authorization is required but missing, return with 401 #94

Closed

gr2m added a commit that referenced this issue Feb 29, 2016

test: validate payload

95860b1

resolves the errors mentiond in #83 * * * This commit was sponsored by Neighbourhoodie You can hire Neighbourhoodie for all your Hoodie / CouchDB / Offline First needs http://go.hood.ie/thehoodiefirm

This was referenced Mar 1, 2016

PATCH /session/account/profile: "404 Admins have no profiles" error #103

Closed

PATCH /session/account/profile -> 409 "data.id must be '{id}'" error #104

Closed

return helpful error message for invalid data.type in payload #91

Merged

This was referenced Mar 19, 2016

POST /accounts Add test for 401, "Session invalid" if user is not an admin #124

Closed

POST /accounts?include=foobar #127

Closed

This was referenced Mar 29, 2016

GET /accounts Add test for 401, "Session invalid" if user is not an admin #136

Closed

Issue/136 #137

Merged

gr2m added a commit that referenced this issue Oct 15, 2016

test: prepared tests based on #83

4a84065

gr2m added a commit that referenced this issue Oct 15, 2016

test: prepared tests based on #83

b83442f

gr2m added a commit that referenced this issue Oct 16, 2016

test: prepared tests based on #83

e3b60d1

gr2m closed this as completed Oct 16, 2016

gr2m removed the in progress label Oct 16, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Inconsistencies between implementation and API spec #83

Inconsistencies between implementation and API spec #83

patriciagarcia commented Feb 18, 2016 •

edited by gr2m

passcod commented Feb 18, 2016

gr2m commented Feb 18, 2016

passcod commented Feb 18, 2016

gr2m commented Feb 18, 2016

breun commented Feb 21, 2016

gr2m commented Feb 21, 2016

gr2m commented Feb 24, 2016

gr2m commented Feb 25, 2016

gr2m commented Feb 28, 2016

POST /requests

GET /accounts

GET /accounts

gr2m commented Oct 15, 2016

gr2m commented Oct 16, 2016

Inconsistencies between implementation and API spec #83

Inconsistencies between implementation and API spec #83

Comments

patriciagarcia commented Feb 18, 2016 • edited by gr2m

Session

PUT /session

PUT /session?include=account.profile

GET /session

DELETE /session

Account

PUT /session/account

GET /session/account

PATCH /session/account

DELETE /session/account

Profile

GET /session/account/profile

PATCH /session/account/profile

Requests

POST /requests

Admins

User account collection

POST /accounts

GET /accounts

GET /accounts/id

PATCH /accounts/id

DELETE /accounts/id

passcod commented Feb 18, 2016

gr2m commented Feb 18, 2016

passcod commented Feb 18, 2016

gr2m commented Feb 18, 2016

breun commented Feb 21, 2016

gr2m commented Feb 21, 2016

gr2m commented Feb 24, 2016

gr2m commented Feb 25, 2016

gr2m commented Feb 28, 2016

POST /requests

GET /accounts

GET /accounts

gr2m commented Oct 15, 2016

gr2m commented Oct 16, 2016

patriciagarcia commented Feb 18, 2016 •

edited by gr2m