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

Allow direct field access in Java Forms #8800

Merged

Conversation

mkurz
Copy link
Member

@mkurz mkurz commented Nov 14, 2018

Fixes https://discuss.lightbend.com/t/databinder-field-access-instead-of-using-accessors/2468

Adds play.forms.binding.directFieldAccess config to switch on direct field access for all forms. To control specific forms only this pull request adds form.withDirectFieldAccess(...) to switch direct field access on or off - or if null falls back to config fallback.

@mkurz mkurz force-pushed the java-forms-direct-field-access branch from cfe0740 to 4406141 Compare November 14, 2018 13:12
Copy link
Member

@marcospereira marcospereira left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made some comments. I wonder if the default for play.forms.binding.directFieldAccess should be true. WDYT?

@mkurz
Copy link
Member Author

mkurz commented Nov 15, 2018

@marcospereira I addressed all your comments plus added two more tests.

I would definitely not change play.forms.binding.directFieldAccess to true by default.

First of all you have to know it's either / or. Either you choose initDirectFieldAccess (which tries to access fields directly) or initBeanPropertyAccess (which tries to use getters and is the default). It's just not possible to mix these two.

These points come into my mind:

  1. If we switch to initDirectFieldAccess=true then the getter methods would not be called anymore - but maybe in some cases such getters are not simple getters but execute logic before returning a value (even if it's only trimming a string before returning it). That logic wouldn't be executed anymore -> not backward compatible.

  2. There may be security problems. Direct field access tries to make private fields accessible via reflection (this calls that). That could cause security exceptions. We don't know which JVMs and/or security managers/settings Play users run their Play apps with, which may don't allow setting (certain) form fields accessible. Again not backward compatible.

  3. I am not 100% sure how that direct field access works in regards to forms. Would it maybe try to access other private fields as well, which are not meant to be form fields and do something with them? I don't know.

As you can see even though I think that our tests would pass (because it looks in our code it's not a problem to make the private fields accessible and we use simple getters without logic everywhere) I would not change the default because that possible would break backward compatibility in user apps.

@marcospereira
Copy link
Member

Thanks for the explanation, @mkurz.

I agree that we should not change the default to true (I was under the impression that the binder was smart enough to handle both cases).

There may be security problems. Direct field access tries to make private fields accessible via reflection (this calls that). That could cause security exceptions. We don't know which JVMs and/or security managers/settings Play users run their Play apps with, which may don't allow setting (certain) form fields accessible. Again not backward compatible.

Hum... I can foresee a warning about illegal reflective access in the future then when using Java 8+. But still, this is a configuration that is off by default. Anyway, it would be good to have a note/warning on our docs about it. Could you add a section on JavaForms that explains the use of directFieldAccess and its consequences?

@mkurz
Copy link
Member Author

mkurz commented Nov 16, 2018

@marcospereira I added docs that show the usage of direct field access and mentions the risk of warnings and exceptions.
Please have a look once more, thanks Marcos!

@mkurz
Copy link
Member Author

mkurz commented Nov 22, 2018

@marcospereira Are the added docs ok for you?

@mkurz mkurz added this to the Play 2.7.0 milestone Nov 23, 2018
Copy link
Member

@marcospereira marcospereira left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Thank you, @mkurz.

@marcospereira marcospereira merged commit c2a4c0a into playframework:master Nov 26, 2018
@mkurz mkurz deleted the java-forms-direct-field-access branch November 26, 2018 16:02
marcospereira pushed a commit that referenced this pull request Nov 27, 2018
* Allow direct field access in Java Forms

* Add tests

* boolean instead of Boolean

* Test for directFieldAccess=false in dynamic form

* Test: When only fields (and no getters) exist but direct field access is disabled

* Document reference.conf property

* Let's be nice - don't break DynamicFormSpec

* Documention for direct field access + possible warning

* Add forgotten parameters
@marcospereira
Copy link
Member

Backport to 2.7.x: a262d90

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

Successfully merging this pull request may close these issues.

4 participants