aip-go icon indicating copy to clipboard operation
aip-go copied to clipboard

Filtering by enums is incorrect by AIP-160

Open zchenyu opened this issue 2 years ago • 6 comments

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 avatar Jan 20 '23 21:01 zchenyu

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.

zchenyu avatar Jan 20 '23 22:01 zchenyu

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

DeclareIdent(fieldName, filtering.TypeString)

I referenced this test code.

newinh avatar Feb 02 '23 13:02 newinh

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).

zchenyu avatar Mar 09 '23 01:03 zchenyu

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

ericwenn avatar Mar 15 '23 09:03 ericwenn

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 https://github.com/aip-dev/google.aip.dev/issues/1039

zchenyu avatar Mar 15 '23 19:03 zchenyu

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

github-actions[bot] avatar Apr 08 '24 08:04 github-actions[bot]