clarify meaning of empty include query param #1532
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
FWIW, I'm proceeding with the assumption that something like this is going to land at some point. |
adilaid
approved these changes
Jan 9, 2021
prathe
reviewed
Jan 13, 2021
_format/1.1/index.md
Outdated
Comment on lines
1240
to
1241
| (U+002E FULL-STOP, ".") list of [relationship][relationships] names. An empty | ||
| value indicates that no related resource should be returned. |
There was a problem hiding this comment.
If we wish to add this clarification.
Suggested change
| (U+002E FULL-STOP, ".") list of [relationship][relationships] names. An empty | |
| value indicates that no related resource should be returned. | |
| (U+002E FULL-STOP, ".") list of [relationship][relationships] names. An empty | |
| value (no characters) indicates that no related resource should be returned. |
There was a problem hiding this comment.
@prathe I'm not sure the clarification (no characters) is needed, but I'm not opposed either. If we were to add this, it should also be added to the similar language around sparse field sets. Since it affects multiple areas, maybe a small separate PR would be best.
auvipy
approved these changes
Jan 13, 2021
dgeb
requested changes
Jan 14, 2021
Co-authored-by: Dan Gebhardt <dan@cerebris.com>
jelhan
commented
Jan 14, 2021
dgeb
approved these changes
Jan 14, 2021
|
Thanks again @jelhan for the clarifying PR and also to @freddrake for raising the issue in #1530. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
The meaning of an empty value for the
includequery parameter was not clear yet. As @freddrake pointed out in #1530 a reader could interpret either as an empty list of relationship paths or as a single (invalid) relationship path.The meaning of an empty value for
fieldsquery parameter is explicitly specified:I think the same applies to the
includequery parameter even though it's not mentioned explicitly yet. Otherwise a client would not be able to tell a server to not include any related resources - even if that server includes some resources by default.I think this is not a breaking change. Let's have a look at several scenarios:
An existing implementation may consider an empty value as a single relationship path. An empty string is not a valid relationship path. The server would be unable to identify the relationship path and response with
400 Bad Request.An existing implementation may silently ignore an
includeparameter if it's value is empty.includeparameter with an empty value is given, the implementation is not compliant with the specification even today. A server must not ignore a givenincludequery parameter. It must response with a 400 if it does not support it.An existing implementation may already interpret an empty
ignoreparameter as an empty list of relationship paths and not include any related resources.The meaning of an empty value for a Sparse Fieldset was also not explicitly specified since a few months ago. It was added in #1500. This was also not considered a breaking change.
Closes #1530