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

fix: Support relationships where both fields have the same name #109

Merged
merged 5 commits into from
Jan 11, 2022

Conversation

AndrewSisley
Copy link
Contributor

Closes #107

Technically two issues here I think:

On the one-many side: Relation.GetField was just returning the first with the same name, making the functionality dependent on golang's randomized map ordering.

On the many-one side: typeJoinMany was just using the schema name + id as the foreign key field name, causing the lookup to fail if the relationship was not named after the other schema.

@AndrewSisley AndrewSisley added bug Something isn't working area/query Related to the query component area/collections Related to the collections system labels Jan 10, 2022
@AndrewSisley AndrewSisley self-assigned this Jan 10, 2022
Copy link
Member

@jsimnz jsimnz left a comment

Choose a reason for hiding this comment

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

Looks great, simple fix for an imporant issue. Cheers

@@ -195,7 +194,6 @@ func (p *Planner) makeTypeJoinOne(parent *selectNode, source planNode, subType *

typeJoin.subTypeName = subTypeFieldDesc.Name
typeJoin.subTypeFieldName = subtypefieldname
typeJoin.rootName = desc.Name // @todo: Correctly handle getting sub/root names
Copy link
Member

Choose a reason for hiding this comment

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

you make a change to this in the named relationships branch as well. Will def need to be rebased, and re-sorted here

for i, f := range r.fields {
if f == field {
if f == field && r.schemaTypes[i] == schemaType {
Copy link
Member

Choose a reason for hiding this comment

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

the key to this fix 👍. Not sure how I missed this originally :/.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is a bit of an edge case and easy to miss if wrestling with the main relationship stuff :)

@jsimnz
Copy link
Member

jsimnz commented Jan 11, 2022

Depends which between this branch and the named relationship branch you want to merge first, as they conflict eachother.

@AndrewSisley AndrewSisley merged commit b9210b9 into develop Jan 11, 2022
@AndrewSisley AndrewSisley deleted the sisley/fix/I107-RelationshipNameCollisionFix branch January 11, 2022 12:48
@orpheuslummis orpheuslummis mentioned this pull request Jan 29, 2022
shahzadlone pushed a commit to shahzadlone/defradb that referenced this pull request Feb 23, 2024
…cenetwork#109)

* Remove unneeded explicit type declaration

* Remove commented out code

* Reduce field.type.name calls

* Remove unused property

* Support relationships between two fields of same name
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/collections Related to the collections system area/query Related to the query component bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Relationship is incorrectly joined when both fields share same name
2 participants