Skip to content

Add field projection to sub-resource/referenced resource queries. #215

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jan 2, 2019
Merged

Add field projection to sub-resource/referenced resource queries. #215

merged 1 commit into from
Jan 2, 2019

Conversation

Dragomir-Ivanov
Copy link
Contributor

@Dragomir-Ivanov Dragomir-Ivanov commented Dec 9, 2018 •
edited
Loading

These potentially can be used in resource hooks and Storer interface implemeters.
No tests breakage. No new test cases added, since I couldn't find this functionality tested in current tests. May add some tests about these in the future.

@Dragomir-Ivanov Dragomir-Ivanov changed the title Add field projection to sub-resource/referenced resource queries. The… Add field projection to sub-resource/referenced resource queries. Dec 9, 2018
smyrman
smyrman previously approved these changes Dec 10, 2018
View reviewed changes
Copy link
Collaborator

@smyrman smyrman left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@rs, does this make sense to you? The main use-case is to make the projection available to hooks, so that e.g. a permission system could error when the user access fields he is not allowed to.

Storer interface implemeters.

As an FIY, this is currently storngly discouraged in the docs.

From https://godoc.org/github.com/rs/rest-layer/resource#Storer:

A storer must ignore the Projection part of the query and always return
the document in its entirety. Documents matching a given predicate might
be reused (i.e.: cached) with a different projection.

@Dragomir-Ivanov
Copy link
Contributor Author

Okay, I may remove that part of the commit message.

smyrman
smyrman previously approved these changes Dec 11, 2018
View reviewed changes
@Dragomir-Ivanov
Copy link
Contributor Author

@smyrman Can we merge that, because it potentially will interfere with #222 .

@Dragomir-Ivanov Dragomir-Ivanov mentioned this pull request Jan 1, 2019
Closed
rs
rs previously approved these changes Jan 2, 2019
View reviewed changes
@rs
Copy link
Owner

rs commented Jan 2, 2019

I think the doc is still relevant for storers. It can make sense for hooks to evaluate projections though.

@Dragomir-Ivanov
Copy link
Contributor Author

Guys, pressed Update branch button here in GitHub, and it generated one "Merged" git commit. I can remove it if necessary.

@smyrman
Copy link
Collaborator

smyrman commented Jan 2, 2019

Yes, let's merge it -- but I need you to do a final rebase before I am allowed to merge it, can you do that?

@Dragomir-Ivanov Dragomir-Ivanov dismissed stale reviews from rs and smyrman via eed81b7 January 2, 2019 19:46
@Dragomir-Ivanov Dragomir-Ivanov force-pushed the master branch 3 times, most recently from 0dd9cb4 to 571d5c4 Compare January 2, 2019 19:55
smyrman
smyrman previously approved these changes Jan 2, 2019
View reviewed changes
@smyrman smyrman merged commit b810f68 into rs:master Jan 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smyrman smyrman smyrman approved these changes

@rs rs rs left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Dragomir-Ivanov @rs @smyrman