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

Filtering by enums is incorrect by AIP-160 #203

Open
zchenyu opened this issue Jan 20, 2023 · 6 comments
Open

Filtering by enums is incorrect by AIP-160 #203

zchenyu opened this issue Jan 20, 2023 · 6 comments
Labels

Comments

@zchenyu
Copy link

zchenyu commented Jan 20, 2023

Hey, I'm trying to understand how this library works for filtering by enums.

In AIP-160, it states

Enums expect the enum's string representation (case-sensitive).

However, it appears that filtering.ParseFilter doesn't accept string for comparing enums.Trying to parse a filter string like state="READY" gives an error like:

check call expr: no matching overload found for calling '=' with [message_type:"my_package.State" primitive:STRING]

Instead, it looks like it uses an ident, as per this example here: https://github.com/einride/spanner-aip-go/blob/master/spanfiltering/transpiler.go#L119 which corresponds to a filter expression like state=READY, which is not conforming to AIP-160.

Am I misunderstanding how this library is used? Or is my assessment correct in that the library incorrectly parsing filters containing enums?

@zchenyu zchenyu changed the title Filtering by enums Filtering by enums is incorrect by AIP-160 Jan 20, 2023
@zchenyu
Copy link
Author

zchenyu commented Jan 20, 2023

One workaround is to avoid using DeclareEnumIdent for enums, and to use DeclareIdent(fieldName, filtering.TypeString) instead.

Then in Transpiler you maintain a map between the enum string values and the enum number values. Then in transpileIdentExpr, you convert the enum ident to a CASE expression to convert the numeric value stored in the database to the string value.

@newinh
Copy link

newinh commented Feb 2, 2023

Try DeclareEnumIdent:) It works for me when using filter string like state = READY.

DeclareIdent(fieldName, filtering.TypeString)

I referenced this test code.

@zchenyu
Copy link
Author

zchenyu commented Mar 9, 2023

The problem is that enum constants must be wrapped in double-quotes as per the spec. This means that READY should be "READY"

Enums expect the enum's string representation (case-sensitive).

@ericwenn
Copy link
Member

I don't agree that string representation means the value should be quoted. I'd say the current behavior is correct.

@zchenyu
Copy link
Author

zchenyu commented Mar 15, 2023

I don't agree that string representation means the value should be quoted. I'd say the current behavior is correct.

Thanks, I just tested on the Google Compute Engine API List API and it accepts both quoted and unquoted. Let me clarify on aip-dev/google.aip.dev#1039

Copy link

github-actions bot commented Apr 8, 2024

This issue has been open for 365 days with no activity. Marking as stale.

@github-actions github-actions bot added the Stale label Apr 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants