Problem/Motivation
Currently views table definitions can only establish joins between tables using a single key pair. They should however also support table joins that require composite keys (a primary key formed by two fields).
Assume a table structure where a table called FOO has a composite key of BAZ_PK and FAZ_PK, and a second table 'BAR' relates to entries in FOO using a composite foreign key of BAZ_FK and FAZ_FK in order to join the two tables together properly, the following sql would be required:
select foo.*
from {foo} foo
inner join {bar} bar on foo.baz_pk=bar.baz_fk AND foo.faz_pk=bar.faz_fk
In that situation, a module developer would not be able to properly declare to view how those tables relate, and data would be mis-reported. This is because the declaration currently would look like this:
$data['bar']['table']['join']['foo'] = array(
'left_field' => 'faz_pk',
'field' => 'faz_fk',
// no place for second key field
);
Drupal core & contrib is mostly not affected by this problem due to the fact that drupal coding and database standards largely encourage the disuse of composite keys, but it is not a requirement. Additionally, there are a number of modules that implement the views api on behalf of other applications, and have little to no control over the data structure therein. Since composite keys are a common structure and organizational construct, it is not uncommon to encounter them in the wild.
At present, there is a solution to this that does not involve a change to views core. Simply put modules that need this functionality must create their own join handler that accepts additional join criteria (not the 'extra' field) and build the join manually. The argument made here is that rather than require many modules to implement what is essentially the same custom class that creates their own arbitrary array/data structure within the views data api standard, the views api should support and handle this need natively.
Also note that this is different from the "extra" construct, which is not designed to re-utilize the joined table, and is specifically designed for extremely rare cases of needing fixed value conditions within a join. this for example would not work for a composite key situation:
$data['bar']['table']['join']['foo'] = array(
'left_field' => 'faz_pk',
'field' => 'faz_fk'
'extra => 'baz_pk=baz_fk'
);
Proposed resolution
There are a number of ways in which tables can be joined:
1) simple 1 key joins
2) composite key joins
3) logic based joins
4) product joins (also called cross joins)
Items 1 and 2 comprise the vast majority of all joins, and are arguably the only two views can reliably handle without developer intervention. At present, only the first type of join is supported. In order to handle something more complex than a simple key=key structure, views should have the proposed modification. Should this patch move forward, users would still be able to use the 'left_field' and 'field' keys, but they would also have the option of expressing the joins in a more semantic format that lends itself to better declarations in the future:
this would be ideal for composite key declarations:
$data['bar']['table']['join']['foo'] = array(
'fields'=>array(
array('left_field'=>'faz_pk', 'field' => 'faz_fk'),
array('left_field'=>'baz_pk', 'field' => 'baz_fk'),
),
);
It would also work for single key declarations:
$data['bar']['table']['join']['foo'] = array(
'fields'=>array(
array('left_field'=>'faz_pk', 'field' => 'faz_fk'),
),
);
Should developers opt for the legacy structure instead for single keys, the patch also allows for that:
$data['bar']['table']['join']['foo'] = array(
'left_field' => 'faz_pk',
'field' => 'faz_fk',
);
Remaining tasks
A test needs to be written that provides a join across two tables using a composite key, and either the sql output is checked to confirm it rendered correctly, or the view is run, and the correct data is output. This, I need help with.
User interface changes
No UI changes
API changes
There is one API addition recommended in this issue. the Addition of a "fields" key to the "join" are of table declarations in views_data(). The proposed structure would look like this:
$data['bar']['table']['join']['foo'] = array(
'fields'=>array(
array('left_field'=>'faz_pk', 'field' => 'faz_fk'),
array('left_field'=>'baz_pk', 'field' => 'baz_fk'),
),
);
It should become the new recommended way of declaring join fields, but not remove the existing way of using 'left_field' and 'field' like so:
$data['bar']['table']['join']['foo'] = array(
'left_field' => 'faz_pk',
'field' => 'faz_fk',
);
Original report by @Mołot
General: Some joins should take care about multiple fields.
My use case:
I have non-base table series with composite PK key (idBrand, idSeries), idBrand is also PK of a base table. And I have series_has_language table, that should be joined to series by series' PK, that means 2 fields. Of course my implementation pretty much assures that idSeries will stay unique, but model's logic does not include this restrain and it should be able to model data like that with views.
Patch should be simple - when generating join, wrap the very ON portion with foreach and glue them with AND. In future someone may find legirimate use-cases for other operators too, but for now I think it's perfectly safe to just ignore them.
Comment | File | Size | Author |
---|---|---|---|
#41 | 1966328-41-8.x_composite_key_support.patch | 3.33 KB | netw3rker |
#35 | views-many-fields-join-1966328-35.patch | 4.78 KB | Mołot |
#33 | 1966328-33.patch | 5.13 KB | Mołot |
#31 | 1966328-31.patch | 5.15 KB | Mołot |
#27 | 0002-http-drupal.org-node-1966328-Fix-strict-warning.patch | 1.13 KB | Mołot |
Comments
Comment #1
Mołot CreditAttribution: Mołot commentedSeems I got it.
Submittimg patch mainly to see what testbot have to say about it, but feel free to post critiques.
Comment #2
Mołot CreditAttribution: Mołot commentedBetter title
Comment #3
Mołot CreditAttribution: Mołot commentedFixed paths. Used git for windows instead of my good old Eclipse. My bad.
Comment #5
Mołot CreditAttribution: Mołot commentedFixed "garbage at line 3" in patch.
Comment #7
Mołot CreditAttribution: Mołot commentedFixing pathes in patch, my mistake.
Comment #9
Mołot CreditAttribution: Mołot commentedOK, trying to fix patch directly i screwed up the php syntax. Sorry all. I guess I'll simply write it agani.
Comment #10
Mołot CreditAttribution: Mołot commentedLet's see.
Seems to work and this time patch applies locally.
I think this functionality is universal enlugh to be added.
Comment #12
Mołot CreditAttribution: Mołot commentedLet's try again, shall we?
Comment #14
Mołot CreditAttribution: Mołot commentedNo endline at the end of file... damn windows git
Comment #15
Mołot CreditAttribution: Mołot commentedComment #16
joachim CreditAttribution: joachim commentedYou seem to have a patch in the patch...
I for one have done some fairly wacky things with Views joins in the past, so I am a bit confused as to what you need that isn't currently possible.
Could you give some examples of the sorts of queries you'd be able to do here that you can't at the moment?
Comment #18
Mołot CreditAttribution: Mołot commentedDammit, I hate working on Windows. I can write code but can't manage patches, it seems :|
About queries not quite possible now - if I have table of manuals, table of products and table of product descriptions for example. To describe product fully and be able to filter by language I need to join descriptions to products by product id, and manuals to descriptions by both product id AND language id. If I don't, I'll end up with all combinations, like russian manual with german description, and will be unable to filter it using one UI control.
Yep, it's not theoretical situation.
Comment #19
joachim CreditAttribution: joachim commented> and manuals to descriptions by both product id AND language id
Sure, there are real use cases.
But can't you do that already with the join 'extra' setting?
Comment #20
Mołot CreditAttribution: Mołot commentedNo I can not. It's buggy now. See: http://drupal.org/node/1966648
And even if I could do that, I still think it doesn't make much sense to describe two idenical conditions in two totally different ways. That may be a matter of taste, but the fact that extra field would need to duplicate code for table aliases and so on is a matter of clarity and safety for me. If we can mitigate code duplication*, why shouldn't we?
*Yes, I know my patch duplicates part of code too. But for now it's only a feature request and I pretty much know how to clean it if this will get a green light from maintainers.
Comment #22
Mołot CreditAttribution: Mołot commentedFixed warning :1510
Comment #23
Mołot CreditAttribution: Mołot commentedComment #24
4d2e CreditAttribution: 4d2e commented#22: many_fields_join_1966328-22.patch queued for re-testing.
Comment #25
4d2e CreditAttribution: 4d2e commentedi can't access my views ui page after i patch the handlers.inc file. do you have any idea, why this happen to me?
Comment #26
Mołot CreditAttribution: Mołot commentedNo idea - I can. Do you use views freshly pulled from GIT as a base for this patch?
The only thing it adds is an option. If you choose not to use it, well, it may move some warnings further in the execution chain, but that's pretty much all...
Comment #27
Mołot CreditAttribution: Mołot commentedPatch to fix strict warning undetected by tests (see http://drupal.org/node/1970408)
By it's very nature it may fail testing itself. If so, will need to merge it with #22 patch to get one that passes tests without unconsciously going around their limits.
Comment #28
Mołot CreditAttribution: Mołot commentedWorks like a charm, no warnings I could find. Tested on php 5.3 and 5.4. Needs patches from both #22 and #27 (at once, I need to combine them into one patch).
Comment #29
Mołot CreditAttribution: Mołot commentedAny chance for further discussion? Anyone?
Especially #19 and #20 - this philosophical part needs a bit more input.
Comment #30
joachim CreditAttribution: joachim commentedCan you post a full patch please -- the latest patch being an interdiff (AFAICT) makes it a bit confusing.
Also, sample core for before and after would help. I am still confused as to what this is fixing and how.
Comment #31
Mołot CreditAttribution: Mołot commentedPatch supplied. Hope I got it OK this time.
The reason there are 2 patches is that the second one only fix warnings that does not affect functionality and was missed by testbot.
Before to join tables on 2 fields you would use:
Mentioned bug: http://drupal.org/node/1966648
After my patch, you can still try to use above syntax, but you can also just write it like this:
Comment #33
Mołot CreditAttribution: Mołot commentedI suck at creating patches. Let's see again.
Comment #35
Mołot CreditAttribution: Mołot commentedComment #36
Mołot CreditAttribution: Mołot commentedI applied #33 on clean views repo, re-exported and it works now. Mystery. But here you go, patch that includes all needed changes as far as I can tell. Not the best one possible, but works, passes tests, and may be the base for further discussion.
Comment #37
joachim CreditAttribution: joachim commentedAh ok, so this isn't about joins in a relationship at all, but the basic hardcoded join that you declare when you declare a new table to Views.
I understand it better now, thanks!
Comment #38
Mołot CreditAttribution: Mołot commentedWell it can be about relationship joins one day. But to do that, you need to have support for it in underlying layer first. So I'm building this from the ground side, not from UI side.
Comment #39
netw3rker CreditAttribution: netw3rker commentedThis is exactly what I was looking for! The whole notion of everything requiring exactly one key, and having to iteratively express composite keys as fixed values was a real problem, this makes it substantially clearer.
My one comment on this is that there are some situations where the composite key is multiple table dependent. In these situations it would make sense for a table syntax like this:
either that, or 'field' should be the unique field name defined in the views definitions somewhere so that it can be looked up, but I think that might be too much of an edge case for this discussion.
Either way, I've applied and tested this patch and it worked exactly as expected! thanks!
Comment #40
netw3rker CreditAttribution: netw3rker commentedPromoting this to RTBC. The community of people who need this and are able to test it is relatively small. The patch in #35 applies cleanly to 3.x-dev, and clears up some extremely ambiguous api functionality in the form of getting rid of the 'extra' field, and defining it with proper keys.
also the issue stagnated for nearly a year, hopefully this will get some fresh eyes on it.
thanks
Comment #41
netw3rker CreditAttribution: netw3rker commentedBumping this to 8.x with the attached patch. tests coming. Can someone review this?
Comment #42
netw3rker CreditAttribution: netw3rker commentedI'm not sure if drupal.org/project/issues/views is the right place to put views 8.x-3.x issues since this is now a core module. This issue has gotten zero attention in over a year, so lets see how DCV goes. I'm moving this issue over to core since there is now a D8 patch ready for testing and input.
Comment #43
joachim CreditAttribution: joachim commentedI think needs its summary updating to explain clearly the use cases, and the proposed change.
Comment #44
netw3rker CreditAttribution: netw3rker commentedComment #45
netw3rker CreditAttribution: netw3rker commentedI've updated the issue summary with a more comprehensive explanation. sorry Mołot I didn't realize it would overwrite your original post and make it look like it was you who wrote it!
ps: d.o guys, thats a little creepy that anyone can do that.....
Comment #46
joachim CreditAttribution: joachim commentedThanks!
The issue summary instructions (https://drupal.org/issue-summaries) have guidelines and also a template that you can use which shows the original post. You can still rescue that from the past revision if you like :)
Comment #47
netw3rker CreditAttribution: netw3rker commentedComment #48
netw3rker CreditAttribution: netw3rker commentedComment #49
netw3rker CreditAttribution: netw3rker commentedComment #50
netw3rker CreditAttribution: netw3rker commented@joachim, Thanks for the link. I've updated the issue to comply with that format, and it looks a lot better now! does this clarify the issue and proposed change better now?
Comment #51
joachim CreditAttribution: joachim commentedLooks very good!
> A test needs to be written that provides a join across two tables using a composite key, and either the sql output is checked to confirm it rendered correctly, or the view is run, and the correct data is output. This, I need help with.
Are there any tables in core that require a join with multiple fields? If not, we'd need to create a dummy test module.
Comment #52
netw3rker CreditAttribution: netw3rker commentedWell, require is relative. AFAIK, there aren't any tables that directly do. The best example I can think of is (in d8) the node -> node_field_data which technically use nid & vid as composite keys. It doesn't really count though since vid is serialized.
I really haven't ever written tests before, so I could use some help with creating this part. does an actual module have to be created that defines all of this, or can the views tests module just get an additional table or two that have this kind of relationship?
Comment #53
joachim CreditAttribution: joachim commentedIf there are no tables in core that have this sort of joining requirement, then yes, this will need a test module that defines a couple of dummy tables.
Comment #55
dawehnerWe need some form of tests here. In general I'm a bit confused because doesn't extra solve the same problem already?
Comment #56
netw3rker CreditAttribution: netw3rker commented@dawehner, 'extra' only allows for specific value extra joins such as "a.nid=b.nid and b.queue_name='something' " it doesn't support composite key joining such as "a.nid=b.nid and a.vid=b.vid". This solves for that.