Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Adjust maxAge minimum & documentation #188
Adjust maxAge minimum & documentation #188
Changes from 10 commits
52062c0
ffffd2a
d2f694d
b47e103
dd519fb
d5f53eb
391a071
ace739b
2409c23
a3ee755
12741fd
2e8350c
e9dc94b
b429ae7
cbe5473
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jlurien I have an issue here because in location-retrieval we do not have unknown/UNKNOWN.
We have either
or
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
400 seems a better fit because it specifically points to maxAge. And it applies for all maxAge values, not only 0.
If maxAge was not provided in the request (any age is acceptible), but the system still cannot find a location, 404 can be used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @alpaycetin74
Works for me and I can update the documentation accordingly.
If not able to provide require freshness (maxAge filled) -->
400
+ codeLOCATION_RETRIEVAL.MAXAGE_INVALID_ARGUMENT
If not able to provide any location for a request without maxAge -->
404
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I didn't realize that.
Regarding errors, to me 400 is for cases when the provided input does not comply with the schema or some constraint that should be known by the client, independently of the device. It could fit for cases where some implementation does not support maxAge=0 for any user (it is still pending how to provide information about limits of the implementation to the client). But for cases where the maxAge cannot be satisfied for a specific device or at a specific moment, I think it is more of a
404
or even422
(the entity combining device and maxAge is not processable by the system).These RESTful interpretations of HTTP statuses are very subtle anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For (2) I think 404 is more natural, the (3) is more about the combination of device + maxAge at a given time
(1)If maxAge input does not fit the pattern (like 5fr/"3) --> 400
(2)If not able to locate the mobile --> 404 with code LOCATION_RETRIEVAL.DEVICE_NOT_FOUND
(3)If not able to provide expected freshness --> 422 with a code LOCATION_RETRIEVAL.UNABLE_TO_FULFILL_MAX_AGE
cc @javier-carrocalabor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another difference with location-verification is that in this case we cannot return a lastLocationTime indicating how old is the more recent location
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hello @bigludo7 ,
Thank you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @alpaycetin74
is maxAge parameter absent in case (2) ?
Humm.. It is fair to assume that yes.
in case (3), there is a maxAge parameter for sure, and it can also be 0, am I right ?
Yes - no doubt for this one !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @bigludo7 ,
if I may summarize my view:
For location retrieval,
(1) Obviously a client error, 400 is appropriate.
It is possible to advise LOCATION_RETRIEVAL.MAXAGE_INVALID_ARGUMENT, but the implementer may not have full control over error messages created in case of syntactic errors - those may be created by software libraries.
(2) No location available, even if maxAge is absent (infinite).
404 with code LOCATION_RETRIEVAL.DEVICE_NOT_FOUND is appropriate.
(3) maxAge condition (including 0) cannot be satisfied.
422 with code LOCATION_RETRIEVAL.UNABLE_TO_FULFILL_MAX_AGE is appropriate.
For location verification,
(1) same
(2) UNKNOWN without lastLocationTime
(3) UNKNOWN, may have lastLocationTime if system has it.
Thank you
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line has to be modified in any case, as the API cannot response "unknown"