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
Add relationships functionality #91
Conversation
- currently just returns the set of relationships and does not query for attributes
Codecov Report
@@ Coverage Diff @@
## master #91 +/- ##
==========================================
+ Coverage 85.06% 85.74% +0.67%
==========================================
Files 38 38
Lines 2210 2315 +105
==========================================
+ Hits 1880 1985 +105
Misses 330 330
Continue to review full report at Codecov.
|
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.
Looking good @ml-evs.
I think the only solution is to, indeed, do a second query for all collected relationships.
As I can see now, this is what is missing?
- Something goes wrong when validating and where AssertionErrors are raised which pollutes all future data to be validated
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.
Haven't gone through your get_included_relationships
function in detail, but it seems legit :)
Co-Authored-By: Casper Welzel Andersen <43357585+CasperWA@users.noreply.github.com>
Co-Authored-By: Casper Welzel Andersen <43357585+CasperWA@users.noreply.github.com>
I think this is basically ready now, the only thing that's not being done is returning a warning if the requested reference was not found under the references endpoint. |
Also changed all but one `assert` statements in tests. The only remaining `assert` statement is in a "real" pytest class.
127d3b9
to
5c6569d
Compare
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.
Seems to be in a state, where it can be merged in and we can call it "version 1" of adding relationships functionality.
Depending on how included
and the include
parameter will be defined finally in the spec, we will need to update this accordingly.
Thanks a bunch @ml-evs !
Yes, I came back to comment that this will currently populate |
Thanks @ml-evs ! |
Closes #71.
references
key as in example spec 6.6.included
key in response with the relationships attached to each entry inside the collection.included
key in response with the reference's attributes, and validate them with reference models in the response.A few notes/queries:
/structures/mpf_1/relationships/references/author
, which I can't see us implementing...BaseResource
is not hashable. As a result, the server code then has to loop over therelationships->references->data
field to filter out duplicates based on ID.links
rather thandata
(JSON API allows both/either,