Skip to content

Adding case insensitive to enum based on loading JsonEnum config#1155

Open
matanshukry wants to merge 10 commits into
google:masterfrom
matanshukry:enum-case-insensitive
Open

Adding case insensitive to enum based on loading JsonEnum config#1155
matanshukry wants to merge 10 commits into
google:masterfrom
matanshukry:enum-case-insensitive

Conversation

@matanshukry

Copy link
Copy Markdown

Fixes issue #927

Problem

Currently when parsing enum we compare thme using the equal operator. Enum values that differ only in cases (e.g. 'sunday' and 'SuNDaY') are not equal, and result in parsing error.
The user should have the possibiliy to decide on the comparator, at least in terms of case insensitive.

Solution

The solution suggested in this PR is:

  1. Add caseInsensitive flag to the JsonEnum class.
  2. Instead of comparing using == operator, we use a comparator function.
  3. Create 2 comparators; the default one, using == operator, and a second one that assumes the value is String, and implementation check if the other object is also string and the .toLowerCase() on both of them is equal.
  4. the comparator function defaultt, if null is provided, is using the == operator.
  5. When creating the call to the enum deserializer method, $enumDecodeNullable or $enumDecode, we add an argumnet to use the 2nd case-insensitive operator, if the caseInsensitive flag exists.

Improvements

  1. Optimization. Currently we go over each value in the map and check it. We can easily use a map, for both the regular case and case-insensitive checks. Not implemented here though as using a map isn't used regardless of the case insensitive case either way.
  2. Extra check for type: currently we don't check the value type is String, but we assume it is like that. We could possibly add that type check, although not sure if that's necessary.

Extra notes

  • I tried adding a test to this PR as well, but I couldn't check that. Specifically it looks like the generated files are bundled and I'm not sure how to generate them. I tried to simply run the generator in the directory, but it results in bigger differences, and I didn't wan to change it manually. I did verify it's working by running this changed code against my own external repo, and ran the generated code.

@matanshukry

Copy link
Copy Markdown
Author

@kevmoo ?

Comment thread json_annotation/lib/src/json_enum.dart Outdated
Comment thread json_annotation/lib/src/json_enum.dart Outdated
Comment thread json_annotation/lib/src/json_enum.dart Outdated
@kevmoo

kevmoo commented Sep 29, 2022

Copy link
Copy Markdown
Collaborator

Happy to look at this again. Will need a rebase! See my other comments!

@matanshukry matanshukry requested a review from kevmoo May 13, 2023 07:47
@matanshukry

Copy link
Copy Markdown
Author

@kevmoo merged with master and fixed the comments. Please take a look again

Comment thread json_annotation/.idea/.gitignore Outdated
@kevmoo

kevmoo commented May 17, 2023

Copy link
Copy Markdown
Collaborator

@matanshukry – you'll need to do a rebase and look at the analyzer errors

@kevmoo

kevmoo commented May 17, 2023

Copy link
Copy Markdown
Collaborator

I think this is headed in a good direction, though!

@matanshukry

matanshukry commented May 18, 2023

Copy link
Copy Markdown
Author

@matanshukry – you'll need to do a rebase and look at the analyzer errors

Currently there are 347 issues - most of which are errors - when I analyze the master branch 😓 (flutter analyze)

Any specific errors you want me to take a look at, that's related to this PR?

@kevmoo

kevmoo commented May 18, 2023

Copy link
Copy Markdown
Collaborator

@matanshukry – you should fix all of them. 😄

@matanshukry

Copy link
Copy Markdown
Author

@matanshukry – you should fix all of them. 😄

What 🤔

Why should I fix all of the errors that exists today in the repo? this PR is about introducing a new fix, not fixing the issues the repo already has.

@kevmoo

kevmoo commented May 18, 2023

Copy link
Copy Markdown
Collaborator

@matanshukry

matanshukry commented May 18, 2023

Copy link
Copy Markdown
Author

@matanshukry – we are green at HEAD - https://github.com/google/json_serializable.dart/actions/runs/4995588862

@kevmoo locally I still a lot of issues, even on master. I'm using dart 3.0.0 so that should be the same, but I am using windows (vs linux on the machines).

You've now let the CI run so I can see the errors through that (and working on it now), but if you have a better way of checking it locally on windows that would be great 🙏

@kevmoo

kevmoo commented May 18, 2023

Copy link
Copy Markdown
Collaborator

@matanshukry – just need to check analyzer, formatter, and run tests locally

@matanshukry

Copy link
Copy Markdown
Author

@kevmoo looks like the result locally are different than the CI though, at least for the analyzer.

For the testing - how do you generate the .g file, specifically for json_test_example.dart ? since it's in the test directory, using the regular build runner doesn't work. Unless I'm missing something.

@matanshukry

Copy link
Copy Markdown
Author

For anyone interested: So apparently the example lib has overrides for path (I tried to just change the version without the override to path, it didn't work). I then copied the example lib (and another common file that's needed), and ran build on that directory.

@kevmoo Regardless, the tests in the annotation won't succeed because it doesn't even recognize the existence of the field caseInsensitive, since it uses the published version and not the path one. I'm assuming that means we need to first publish the annotation version, and then upgrade the serializable to use the annotation.

Hence I've created a separate PR for the annotation only:
#1323

Let me know if there's another method you would like to do that.

@kevmoo

kevmoo commented May 20, 2023

Copy link
Copy Markdown
Collaborator

Just add (path) dependency overrides. You want to test EVERYTHING before you publish ANYTHING.

@matanshukry

Copy link
Copy Markdown
Author

Just add (path) dependency overrides. You want to test EVERYTHING before you publish ANYTHING.

Already did that locally, that's not the issue. All tests passed when I do that.

I'm assuming you don't want that added to this PR though.

@matanshukry

Copy link
Copy Markdown
Author

@kevmoo I can see you allowed the build to run.
As expected we can see it failed on the tests that require data from the 2nd repo.

So next step would be to review merge and publish the 2nd PR - #1323 .

Let me know if there's anything else you want me to do in the meantime.

@matanshukry

Copy link
Copy Markdown
Author

@kevmoo fyi - there's no reason to run the builds until the other PR is merged and the versions are updated; it will always fail until then.

@petrnymsa

Copy link
Copy Markdown

Hi @matanshukry @kevmoo Do you plan to continue on this? 🙏 I'd love to have support for enum case insensitivity.

I am open to help with finishing this PR as well.

@matanshukry

Copy link
Copy Markdown
Author

Been some time, but trying to get back into this and complete it. Honestly I can't get the code generation to run locally, in order to actually work with it.

I took latest master branch, went into json_serializable directory and tried to build ( dart run build_runner build --delete-conflicting-outputs), and got multiple errors: https://gist.github.com/matanshukry/69dc5ee32bb7dcd4801e752c850f3f22

I also tried to look at past builds, and take the hash of a successful one, e.g. e1381aa217895bb9ed804447a3979a2eaf01e3d0 ( e1381aa )
Still fail, same errors.

Can't really work on it until I get it done
@kevmoo would love your help on that

@kevmoo

kevmoo commented Aug 4, 2025

Copy link
Copy Markdown
Collaborator

Make sure you upgrade dependencies locally.

@matanshukry

Copy link
Copy Markdown
Author

Make sure you upgrade dependencies locally.

You mean flutter pub upgrade ? If so I did that, doesn't help. Same errors.

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