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

IdentityCache compiler misses a fetch_multi_* #1754

Closed
bitwise-aiden opened this issue Jan 9, 2024 · 3 comments · Fixed by #1758
Closed

IdentityCache compiler misses a fetch_multi_* #1754

bitwise-aiden opened this issue Jan 9, 2024 · 3 comments · Fixed by #1758
Labels
bug Something isn't working

Comments

@bitwise-aiden
Copy link
Contributor

Description

When using cache_attribute with multiple by fields, the resulting fetch_multi_field_a_by_field_b_and_field_c method is not created.

It appears as though there is a length check that is preventing this from being generated. Looking back to where it appears to be introduced, there isn't clear indication of what this check was intended for.

One approach to solving could be removing the check and seeing what breaks, or adding a create_attribute_fetch_by_methods method to create for this case.

@bitwise-aiden bitwise-aiden added the bug Something isn't working label Jan 9, 2024
@KaanOzkan
Copy link
Contributor

There wasn't support for composite keys until Shopify/identity_cache#534. The compiler will have to be updated to reflect these changes.

@bitwise-aiden
Copy link
Contributor Author

Yup! Is the preferred adding a new create_attribute_fetch_by_methods to handle this? The length check in the compiler predates the cache_attribute addition, so I don't want to mess with that.

@KaanOzkan
Copy link
Contributor

Looking at the PR diff I think removing the length check will be enough. If you're going ahead with the solution I'd say let's remove it and then add new tests and see. I don't have IDC context.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants