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
merged 5 commits into from Jun 4, 2018

Conversation

Projects
None yet
2 participants
@pimterry
Member

pimterry commented May 29, 2018

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

@resin-io-versionbot

This comment has been minimized.

Contributor

resin-io-versionbot bot commented May 29, 2018

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

@pimterry

This comment has been minimized.

Member

pimterry commented May 29, 2018

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

@Page-

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- Jun 4, 2018

@Page-

Page- approved these changes Jun 4, 2018

@resin-io-versionbot resin-io-versionbot bot merged commit d6c8b04 into master Jun 4, 2018

7 checks passed

AutoMerges PR merging is in progress
Reviewers 1/1 review approvals met
Versionist Found all required commit footer tags
ci/circleci: node-10 Your tests passed on CircleCI!
Details
ci/circleci: node-4 Your tests passed on CircleCI!
Details
ci/circleci: node-6 Your tests passed on CircleCI!
Details
ci/circleci: node-8 Your tests passed on CircleCI!
Details

@resin-io-versionbot resin-io-versionbot bot deleted the 18-fix-fk-rule-field-name branch Jun 4, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment