[
https://issues.apache.org/jira/browse/LANG-965?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14111332#comment-14111332
]
Andre Diermann commented on LANG-965:
-------------------------------------
Hi Benedikt,
bq. Maybe we should add a comment in JavaDoc, that users have to take care of
restoring accessibility when using getField()...
Yes, adding a comment in the corresponding JavaDoc is a good idea and will
raise the attention for a potential "accessibility leak" a bit more. Should I
change the JavaDoc accordingly as part of this issue or should this be a
dedicated issue and patch?
bq. However I think it would be better to use FieldUtils.readField() to check
if the field has been set correctly and to assert that the field is not
accessible after the call to writeField(). WDYT?
I think the author of the original test code wanted to use "pure" Java for the
assertion. The correct behavior of {{Field.get()}} is guaranteed by the JDK
while {{FieldUtils.readField()}} is not (since this method is tested in the
same test class and execution order of jUnit tests might be random). So from my
understanding the avoidance of a method of the class under test to test another
method within the class under test is on purpose. So my intention here was
really to use the minimum amount of changes to the original code to keep the
tests green and this one-liner was the shortest solution.
Maybe this is a smell that tests for the correct restoring of the accessibility
is missing? O:) I will add further tests to assert that accessibility is
restored correctly. This should not be tested implicitly within the
{{testWrite…()}}. Ok?
bq. 1. missing curly brackets ;-) We use curly brackets even for one line
blocks.
Sorry, you are write. I will change this.
bq. 2. I'm not sure whether {{!Modifier.isPublic(field.getModifiers()))}}
really is the same as {{Field.isAccessible() == false}}. Reading through the
JavaDoc of {{Field.setAccessible(boolean)}} sounds like it is a difference. But
I don't know. It think it would be better to get the field via {{getField(cls,
fieldName, false)}}, store the accessibility, do all neccessary operations and
then restore the original state. This way you only need to restore the
accessibility once.
The reason for the usage of {{Modifier.isPublic()}} is the used workaround for
JDK 1.3 in {{getField()}} (see line 107). The accessibility of the field is
only set to true, when forceAccess is true and the modifier is public. That’s
why I need to check both before evantually restoring accessibility to false.
But I totally agree with you, that it looks confusing. I don’t know if we can
solve this without jeopardizing the workaround.
Sorry if the patch is that confusing, but I really spent a lot of time to
understand the original code and then tried to use a minimum amount of changes
to fix issue #965. Thus, I am afraid some of your remarks address the original
code as well. :-/
See you,
André
> FieldUtils methods leak accessible flags
> ----------------------------------------
>
> Key: LANG-965
> URL: https://issues.apache.org/jira/browse/LANG-965
> Project: Commons Lang
> Issue Type: Bug
> Components: lang.reflect.*
> Affects Versions: 3.1, 3.2.1
> Environment: Apache Maven 3.1.1
> (0728685237757ffbf44136acec0402957f723d9a; 2013-09-17 11:22:22-0400)
> Maven home: C:\Java\apache-maven-3.1.1\bin\..
> Java version: 1.7.0_51, vendor: Oracle Corporation
> Java home: C:\Program Files\Java\jdk1.7.0_51\jre
> Default locale: en_US, platform encoding: Cp1252
> OS name: "windows 7", version: "6.1", arch: "amd64", family: "windows"
> Reporter: Gary Gregory
> Assignee: Benedikt Ritter
> Fix For: Review Patch
>
> Attachments: commons-lang-965.patch
>
>
> When various FieldUtils methods are called the accessible is set to true but
> never reset to false. This is side-effect should be cleaned up.
> This makes a mess of the object model which represents the class meta data.
--
This message was sent by Atlassian JIRA
(v6.2#6252)
