timbunce
Feature/join
Coverage increased (+2.0%) to 88.68% when pulling 396b9b8bf0f46bfc13657f1c666ac2299183b039 on feature/join into 840d9e7bf275d0f1fd44443fa595566e31dbdb2a on master.
Coverage increased (+2.0%) to 88.68% when pulling 396b9b8bf0f46bfc13657f1c666ac2299183b039 on feature/join into 840d9e7bf275d0f1fd44443fa595566e31dbdb2a on master.
Coverage increased (+2.04%) to 88.72% when pulling 396b9b8bf0f46bfc13657f1c666ac2299183b039 on feature/join into 840d9e7bf275d0f1fd44443fa595566e31dbdb2a on master.
For the record I'm uncomfortable about a few aspects of this:
- the '# Prevent joins calling the DB for no cached relations in the join statement' checks suggest a poor approach
- the reuse of _handle_prefetch_param to handle join
- the removal of the _pre_update_resource_method mechanism without explanation or replacement (which probably points to the lack of test coverage for what the mechanism does re allowing code to be shared between media types).
- cuddling the else's :)
More fundamentally though, I'm unclear about what the semantics of join=... should be.
Given GET /cd?join=artist&artist.name=Random+Boy+Band the join seems redundant because it's implied by the artist. prefix of artist.name=...
Similarly for cds in GET /artist/1?join=cds&order=cds.cdid
You'd prefer thay any required joining be infered by use of the relation names? can't say I care either way really..
What happens if there are multiple ways to join table A to B though? (if A and B are several levels apart..) Lets say for (almost realy) argument: /user?pantry.name=fred .. where user has_many pantries belongs_to pantry.. AND user belongs_to family has_many pantries belongs_to pantry ... how did you know which chain I meant?
- The '#prevent ...' comment is as a result of piggy backing on the back of the prefetch handling. This is largely done because to DBIC prefetch and Join are basically the same thing. I agree this is a little clunky.
- we reuse the _handle_prefetch_param to handle joins because they have the same structure and syntax. IIRC this is the name of the function in DBIC that this code was stolen from. Maybe we should rename it? to what?
- I don't think we can infer the join relation from the relation prefix as we can't predict the depth of that easily. Maybe we can allow infered joins on first level relations.
The syntax for join should be identical to that of prefetch. Prefetch just effectively adds a columns arg to force returning of the related resources.
RE: Cuddling else. That fine though its probably worth having a wider discussion on code standards
The removale of the _pre_update_resource_method should have been seperate to this work....Woops. Basically we seemed to be providing a pre update hook when the resource can just use method modifiers to achieve the same thing.
@ungrim97 re _pre_update_resource_method I don't think method modifiers can be used to achieve the same thing. In apps that support multiple media types they'll be multiple method modifiers that are unaware of each other or which should be 'active' for the media type of the current request. I think this is another aspect of the need for abstracting adaptors and serializers. Anyway, off-topic for this PR. I'll comment on the join param later today - gott'a go now.
@castaway,
What happens if there are multiple ways to join table A to B though? (if A and B are several levels apart..) Lets say for (almost realy) argument: /user?pantry.name=fred .. where user has_many pantries belongs_to pantry.. AND user belongs_to family has_many pantries belongs_to pantry ... how did you know which chain I meant?
because the 'pantry' in pantry.name=... is the name of the relation to use. (Best to avoid 'table' in discussions).
Note that I have relatively narrow actual experience with DBIC and WAPID so I could easily be going down blind alleys and barking up wrong trees. So do push back, ideally with examples, if I don't seem to be talking sense.