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 "model does not have column" error #850

Merged
merged 1 commit into from
Sep 10, 2023

Conversation

martoche
Copy link
Contributor

@martoche martoche commented Jun 6, 2023

The following type has two belongs-to fields with different names (BillingAddress and ShippingAddress) but with the same type (Address):

	type CompanyAddress struct {
		Id                int `bun:",pk"`
		CompanyId         int
		BillingAddressId  int
		BillingAddress    *Address `bun:"rel:belongs-to,join:billing_address_id=id"`
		ShippingAddressId int
		ShippingAddress   *Address `bun:"rel:belongs-to,join:shipping_address_id=id"`
	}

It causes this error when making a SELECT (see test case):

sql: Scan error on column index 7, name "company_address__shipping_address__id": bun: model=Company does not have column=company_address__shipping_address__id

I've attached a test case to demonstrate the problem.

This commit is an attempt to fix the bug. I've narrowed it down to the Table.inlineFields method.

There is a seen map that saves which fields have already been visited to avoid an infinite loop in case of circular relations. In the case with two fields with different names but the same type (BillingAddress and ShippingAddress), the seen map prevents the inlineFields method to visit one of the two fields, which causes the error.

This is just one attempt to fix the bug, I'm happy to make changes if you think it should be fixes in another way.

(Another attempt by another contributor to fix this bug has been made in #849, original bug #847. This other PR doesn't have a test case, but fixes the problem elsewhere in the code, it might be interesting to compare the two different fixes.)

@enzalito
Copy link

enzalito commented Jun 6, 2023

Hi, thank you for this PR.

I wanted to inform you that there is a flaw in the way you're checking if a field has already been visited.
With nested relations, fields with the same type and the same name can be involved in a request. In that case, the bug will still happen.
You can use the reproducer I gave in #847 to see for yourself.

Storing the index of the field in inlineFieldsSeenKey instead of its name should fix the problem. It is a slice though so, if you choose this approach, stringifying it will be necessary.

@martoche martoche force-pushed the fix-model-does-not-have-column branch from ef809d7 to 4fd3638 Compare June 7, 2023 05:48
@martoche
Copy link
Contributor Author

martoche commented Jun 7, 2023

Thanks for your input @TonDar0n, I've integrated your test case and fixed the code accordingly.

This fixes the case of two belongs-to fields with different names
but with the same type.
@martoche martoche force-pushed the fix-model-does-not-have-column branch from 4fd3638 to cda3343 Compare June 9, 2023 15:35
@martoche
Copy link
Contributor Author

martoche commented Jun 9, 2023

I've just force-pushed a new version, adding an new test case for circular relations.

The seen map was previously shared across all calls to inlineFields, stopping the recursion as soon as a field with a previously seen type was visited.

Now, the seen map is cloned so that all the paths of what has been visited is saved, and we're able to stop the recursion more precisely: only when a path starts looping.

@vmihailenco this is ready for review, thanks!

@vmihailenco
Copy link
Member

Merging since this is an improvement to the current behavior, but this does not look like a complete fix for the recursion case. We just give up at some later point then before.

#849 looks more promising since it adds dynamic resolving, but it requires more work...

@vmihailenco vmihailenco merged commit 16367aa into uptrace:master Sep 10, 2023
@calorie calorie mentioned this pull request Sep 15, 2023
vmihailenco added a commit that referenced this pull request Sep 16, 2023
@vmihailenco
Copy link
Member

This is reverted by 387228e due to #897

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.

3 participants