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

Exclude field condition #2066

Closed
VladDrake opened this issue Jan 27, 2022 · 3 comments · Fixed by #2121
Closed

Exclude field condition #2066

VladDrake opened this issue Jan 27, 2022 · 3 comments · Fixed by #2121
Labels

Comments

@VladDrake
Copy link

https://github.com/google/gson/blob/master/gson/src/main/java/com/google/gson/internal/bind/ReflectiveTypeAdapterFactory.java#L68

Is it ok? If follow that logic, the field going to be excluded when
A. NOT exclude a class
AND
B. NOT exclude a field

I guess it should be return excluder.excludeClass(f.getType(), serialize) || excluder.excludeField(f, serialize)

that means, that, the field going to be excluded when
A. exclude a class
OR
B. exclude a field

Correct me if I wrong

@VladDrake VladDrake added the bug label Jan 27, 2022
@Marcono1234
Copy link
Collaborator

You are right, that does not look correct. It seems the method name is incorrect (should probably be includeField), see its usage below:

boolean serialize = excludeField(field, true);
boolean deserialize = excludeField(field, false);
if (!serialize && !deserialize) {
continue;
}

Changing the implementation of the method causes a lot of test failures because it would erroneously exclude fields then.


Note that I am not a member of this project.

@eamonnmcmanus
Copy link
Member

Agree with @Marcono1234, I think the two excludeField methods just need to be renamed. Feel free to send a PR.

@Marcono1234
Copy link
Collaborator

Maybe the static excludeField can also be removed and the instance method could directly perform the checks. Also it looks like this instance method could be made private.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants