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(doctrine): fixed backed enum filter to use case name as value to filter by #6864

Open
wants to merge 1 commit into
base: 3.4
Choose a base branch
from

Conversation

psihius
Copy link
Contributor

@psihius psihius commented Dec 12, 2024

Q A
Branch? 3.4
Tickets #6298
License MIT
Doc PR

In GraphQL enums are serialized using the case name, not the value of the enum in the responses. Backed Enum Filter users value as enum values, which is also hard coded to "string" type, which creates issues with backed enums of any other type than string (I run into this issue using a int backed enum).

This straightforward fix puts the expected filter values inline with the serialization of enums, so now you can use the GraphQL schema to populate your enum filter (schema defined enums as ENUM_NAME: ENUM_NAME and not ENUM_MANE: ENUM_VALUE )

One problem I do have is I can't make the schema to define the filter as Enum so it shows up as one in GraphQL schema, I'm probably missing something here.

@psihius psihius changed the title fix(doctrine): fixed backed enum fiter touse case name as value fix(doctrine): fixed backed enum filter to use case name as value to filter by Dec 13, 2024
@soyuka soyuka requested a review from alanpoulain December 13, 2024 13:06
@soyuka
Copy link
Member

soyuka commented Dec 13, 2024

can you rebase please? Not sure if this isn't breaking current implementations though

@psihius psihius force-pushed the bugfix/backed-enum-filter-cases branch from 08f842e to 372c9c0 Compare December 13, 2024 20:03
@psihius psihius force-pushed the bugfix/backed-enum-filter-cases branch from 372c9c0 to bd40751 Compare December 13, 2024 20:05
@psihius
Copy link
Contributor Author

psihius commented Dec 13, 2024

can you rebase please? Not sure if this isn't breaking current implementations though

Done.

Probably would break custom implementations where people just accounted for this problem, but the current state basically means that filtering an enum, unless it's a string, just does not work at all. String based filtering I imagine might break, but then people could not relly on schema provided enumumeration and had to populate UI fields with static values or some javascript shenanigans.

I've tested only with GraphQL, I sadly don't have time to try and play around with REST and whatever other versions there are. I also looked at the test cases for the filter - they are small and cover basically just string backed enum only, so i don't know what even the expectations here are, if any at all :D

@soyuka
Copy link
Member

soyuka commented Dec 16, 2024

it looks like it breaks our tests, I'm surprised that this doesn't work as expected though.

@psihius
Copy link
Contributor Author

psihius commented Dec 17, 2024

it looks like it breaks our tests, I'm surprised that this doesn't work as expected though.

The last 2 tests breaking makes sense - it's just case sensitivity when comparing strings, fixable.

The other tests, that check if the generated DQL query matches, now that's a head-scratcher.

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.

2 participants