Exclude static and transient fields from form fields by default about spring-mvc-test-utils HOT 8 CLOSED

Hi @schosin,

Thanks for your proposal.

I agree, this functionality should, at least, be configurable.

As you mentioned, a simple fix would be (l. 142):

.filter(field -> !field.isSynthetic() && !Modifier.isTransient(field.getModifiers()) && !Modifier.isStatic(field.getModifiers()))

However, to make it configurable, would it make sense (according to your usage) to overload the postForm method with a new Predicate<Field> parameter to exclude the ones matching the given predicate?

The library would include a non-synthetic Predicate by default and we could rename the getAllNonSyntheticFields(Object form) method to getAllFields(Object form, Predicate<Field> fieldsToExclude).

    private static List<Field> getAllFields(Object form, Predicate<Field> fieldsToExclude) {
        return FieldUtils.getAllFieldsList(form.getClass())
                .stream()
                // Only retrieve non synthetic fields to ignore JaCoCo's ones (https://github.com/f-lopes/spring-mvc-test-utils/issues/10)
                .dropWhile(fieldsToExclude)
                .collect(Collectors.toList());
    }

This way, any client could exclude any unwanted field(s) by adding a Predicate<Field> parameter.

What do you think of this proposal?

Of course, feel free to submit a pull request/discuss here to promote your use case/implementation.

Regards,

from spring-mvc-test-utils.

While such a predicate grants the most power to the user, it also forces them to deal directly with reflection, which I try to avoid in application code as much as possible.

I would suggest a configuration parameter instead that could also be used to configure additional behaviour in the future without adding more parameters. Another use case could be to overwrite propertyEditors per call or to exclude fields by name/class, but that would be out of scope of this issue.

For this I'm thinking about something along the lines of:

public static class Configuration {
    private final Predicate<Field> fieldPredicate;

    public static Builder ofDefaults() { return builder().includeSynthetic(false) }
    public static Builder builder() { return new Builder() }

    public static class Builder {
        // fields

        public Builder fieldPredicate(Predicate<Field> predicate) { ... } // takes precedence
        public Builder includeStatic(boolean include) { ... }
        public Builder includeTransient(boolean include) { ... }
        public Configuration build() { ... } // uses fieldPredicate if present, otherwise constructs from other parameters
    }
}

The default configuration would be Configuration.ofDefaults().build() to keep backwards compatibility.

Greetings

Edit: I removed the "includeSynthetic" flag since that causes StackOverflowExceptions. I don't think there is a need to include such fields anyway.

from spring-mvc-test-utils.

I pushed a commit in my fork that implements my suggestion. I also included a test testSyntheticFieldExcluded since there was no such test yet.

Configuration.ofDefaults() includes both static and transient fields, while Configuration.builder() does not. I still think it is strange to include these fields by default, but that way there'd be no breaking changes, in case anyone depends on these fields being included in their tests. 🤷‍♂️

Any thoughts on my changes before I create a pull request?

from spring-mvc-test-utils.

Hi @schosin,

Thanks for your great suggestion, again! Indeed, it will allow making more configurations available to clients in the future while allowing them to provide a custom one.

Thanks for your test addition as well.

Wouldn't it be better/clearer (reading-wise) to build a composite predicate if includeStatic or includeTransient are enabled (without omitting the custom one if provided)?
Probably something like that in the buildFieldPredicate method:

if (this.includeStatic) {
    this.fieldPredicate = this.fieldPredicate.and(field -> Modifier.isStatic(field.getModifiers()));
}
if (this.includeTransient) {
    [...]
}

I recognize this version is slightly more verbose. However, I think it's more readable and would provide more clarity/regularity when adding more predicates in the future.

What do you think?

This is a great suggestion. Thanks again!

from spring-mvc-test-utils.

I thought about chaining predicates as well. If you also think it's better, I will change that. But I will probably use method references instead as those are a lot easier to debug than a lambda:
this.fieldPredicate = this.fieldPredicate.and(Builder::isNotStatic)

But now the question remains, whether the custom fieldPredicate should include the !includeX-predicate or not. It's probably better to combine them so a user doesn't have to implement the !static/!transient checks if a custom fieldPredicate is configured.

Configuration.builder().includeTransient(true).fieldPredicate(field -> field.getName().startsWith("prefix")) should also include private transient String prefixField in my opinion.

While testing these changes, I also updated the project, changing the JDK from 8 to 11. Now I also run into the same problems in the tests with the comparator I added to ConfigurationForm:
java.lang.reflect.InaccessibleObjectException: Unable to make field private final java.util.function.Function java.util.Comparator$$Lambda$239/0x0000000800c3b068.arg$1 accessible: module java.base does not "opens java.util" to unnamed module @6a1aab78

The problem only arises when running with JDK 14-16 and above (16 for sure, 13 not. Don't have any JDKs in between). I would still argue that static fields shouldn't be included since those fields wouldn't be populated when posting forms to a Spring controller anyway. Would it be acceptable to change the base predicate from !field.isSynthetic() to !field.isSynthetic() && !Modifier.isStatic(field.getModifiers())?

from spring-mvc-test-utils.

I thought about chaining predicates as well. If you also think it's better, I will change that. But I will probably use method references instead as those are a lot easier to debug than a lambda:

Of course, this was just an example.

But now the question remains, whether the custom fieldPredicate should include the !includeX-predicate or not. It's probably better to combine them so a user doesn't have to implement the !static/!transient checks if a custom fieldPredicate is configured.

I wouldn't add these predicates by default (e.g do not include static/transient/synthetics fields).

Also, wouldn't it be more natural to use the positive form (e.g. include rather than exclude)?

 public Configuration build() {
    if (this.includeTransient) {
        this.fieldPredicate = this.fieldPredicate.and(Builder::isTransient);
    }

    return new Configuration(this.fieldPredicate);
}

Configuration.builder().includeTransient(true).fieldPredicate(field -> field.getName().startsWith("prefix")) should also include private transient String prefixField in my opinion.

It would give the client more flexibility, but this is not necessary right now, in my opinion, unless you need it.

While testing these changes, I also updated the project, changing the JDK from 8 to 11. Now I also run into the same problems in the tests with the comparator I added to ConfigurationForm:
java.lang.reflect.InaccessibleObjectException: Unable to make field private final java.util.function.Function java.util.Comparator$$Lambda$239/0x0000000800c3b068.arg$1 accessible: module java.base does not "opens java.util" to unnamed module @6a1aab78

I didn't encounter this error at all using JDK 11 or 18 (based on the latest commit on your branch):

docker run --rm -it -v $(pwd):/workspace -w /workspace maven:3.8.6-jdk-11-slim mvn clean test

docker run --rm -it -v $(pwd):/workspace -w /workspace maven:3.8.6-openjdk-18 mvn clean test

The problem only arises when running with JDK 14-16 and above (16 for sure, 13 not. Don't have any JDKs in between). I would still argue that static fields shouldn't be included since those fields wouldn't be populated when posting forms to a Spring controller anyway. Would it be acceptable to change the base predicate from !field.isSynthetic() to !field.isSynthetic() && !Modifier.isStatic(field.getModifiers())?

Of course!

Thanks again for all your suggestions/contributions🙏

from spring-mvc-test-utils.

I wouldn't add these predicates by default (e.g do not include static/transient/synthetics fields).

Will change that. Since Configuration.builder() and Configuration.ofDefaults() would behave the same way now, I will remove the latter.
Just to clarify, I removed the configuration option for static because Spring doesn't fill such fields anyway. There's even an argument to exlcude final fields as well since these fields aren't populated by Spring as well. Thoughts?

Also, wouldn't it be more natural to use the positive form (e.g. include rather than exclude)?

this.fieldPredicate = this.fieldPredicate.and(Builder::isTransient); would include only transient fields though. That's why I have the double negation. If includeTransient is false, fields must not be transient. If includeTransient is true, no filter on transient modifier are applied as we want both transient and non-transient fields.

I didn't encounter this error at all using JDK 11 or 18 (based on the latest commit on your branch):

With the commit you tested, I excluded static fields altogether, so there's no attempt to use reflection on that comparator anymore. If you want to reproduce the issue, you would have to make that field non-static (and non-final I guess). Not that it makes sense to do that with a comparator.

from spring-mvc-test-utils.

Will change that. Since Configuration.builder() and Configuration.ofDefaults() would behave the same way now, I will remove the latter.
Just to clarify, I removed the configuration option for static because Spring doesn't fill such fields anyway. There's even an argument to exlcude final fields as well since these fields aren't populated by Spring as well. Thoughts?

It makes sense, thanks for your research and your modifications.

this.fieldPredicate = this.fieldPredicate.and(Builder::isTransient); would include only transient fields though. That's why I have the double negation. If includeTransient is false, fields must not be transient. If includeTransient is true, no filter on transient modifier are applied as we want both transient and non-transient fields.

You're right. I quickly read the code without going into the semantics.

With the commit you tested, I excluded static fields altogether, so there's no attempt to use reflection on that comparator anymore. If you want to reproduce the issue, you would have to make that field non-static (and non-final I guess). Not that it makes sense to do that with a comparator.

I've been able to reproduce it using JDK 18 but not using JDK 11. At this time, this library doesn't support JDK 18. However, I've been able to fix it using this Stack Overflow thread.
Just add this configuration to the maven-surefire-plugin:

<argLine>
    --add-opens java.base/java.lang=ALL-UNNAMED    
    --add-opens java.base/java.util=ALL-UNNAMED
</argLine>

I am not very knowledgeable on this subject so I can't recommend it. Also, clients would probably have to add this configuration too to make the library work so it's not optimal in my opinion.

from spring-mvc-test-utils.