Skip to content
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

Ensure the correct FK field name is used in link table rules #19

Merged
5 commits merged into from
Jun 4, 2018

Conversation

pimterry
Copy link
Contributor

This code assumes that the FK from a link table to another table has the same name as the target table. That's sometimes true, but not always - nowadays we include the verb in there. This breaks some parts of the billing model.

This code now searches the fields for a FK that connects to the correct table, which fixes this issue.

There's a further extension to this where we find the FK for the correct verb, since there could be multiple. I haven't looked at that because a) it doesn't block me and b) I can't see an easy way to connect the fields to the verbs without writing special logic to parse verb & field names, which seems pretty nasty. Any ideas?

Connects to #18
Change-Type: patch

@pimterry pimterry requested a review from Page- May 29, 2018 10:38
@ghost
Copy link

ghost commented May 29, 2018

@pimterry, status checks have failed for this PR. Please make appropriate changes and recommit.

@pimterry
Copy link
Contributor Author

Forgot to update the tests correspondingly - now fixed (though I question the value of a test suite that needs bug fixes that are near-identical to the real code...)

@pimterry pimterry requested a review from nazrhom May 30, 2018 15:45
Copy link
Contributor

@Page- Page- left a comment

Choose a reason for hiding this comment

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

It took a bit of time to figure out how to resolve the relationship correctly in the lf-to-abstract-sql context (it's only been used in other contexts previously), but it turns out it's actually really simple, here's the code I used:

	LinkTable :actualFactType =
		RoleBindings(actualFactType):binds
		LinkTableAlias(binds, actualFactType):tableAlias
		GetTable(actualFactType):linkTable
		{['SelectQuery', ['Select', []], ['From', [linkTable.name, tableAlias]]]}:query
		{	_.each(binds, function(bind, i) {
				var baseIdentifierName = actualFactType[i*2][1],
					targetTable = $elf.GetTable(baseIdentifierName);
				if(!targetTable.primitive) {
					var relationshipMapping = $elf.relationships[linkTable.name][baseIdentifierName].$;
					$elf.AddWhereClause(query,
						['Equals',
							['ReferencedField', tableAlias, relationshipMapping[0]],
							['ReferencedField', bind.binding[1], relationshipMapping[1][1]]
						]
					);
				}
			})
		}
		-> ['Exists', query],

Basically a relationship mapping is [ $fromField, [ $toTable, $toField ] ] (or just [ $fromField ] if it's a primtiive attribute - which it won't be here). If you could check that out and see if it still works for you that'd be great, it passes the tests at least

@pimterry pimterry requested a review from Page- June 4, 2018 11:17
@ghost ghost merged commit d6c8b04 into master Jun 4, 2018
@ghost ghost deleted the 18-fix-fk-rule-field-name branch June 4, 2018 14:35
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants