[
https://issues.apache.org/jira/browse/LANG-965?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14112076#comment-14112076
]
Benedikt Ritter commented on LANG-965:
--------------------------------------
Hello André,
bq. Should I change the JavaDoc accordingly as part of this issue or should
this be a dedicated issue and patch?
a new issue for this small change is a bit overkill. Just add it to your patch.
bq. 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 [...]
yes, but correctness of {{readField()}} is asserted by individual unit tests
for that method, so I think it's okay to use it in the test for
{{writeField()}} if this makes the test code easier to understand.
bq. I will add further tests to assert that accessibility is restored
correctly. This should not be tested implicitly within the testWrite…(). Ok?
I'm fine with what ever apporach you take ({{Field.get() or readField()}}, as
long as we have have test code that really asserts that the accessibility is
restored.
bq. The reason for the usage of Modifier.isPublic() is the used workaround for
JDK 1.3 in getField() (see line 107).
I'm not sure I've mae myself clear enough. I'll try to explain better :-) My
critique is, that the code for restoring accessibility is duplicated, although
the one {{writeField()}} calls the other. So the idea is use {{getField}} with
forceAccess = false to retrieve the field. This will return the field, even if
it is not accessible. After that you can simply call the other {{writeField()}}
and pass the forceAccess parameter into it. That method in turn will handle
restoring of the accessibility flag for you.
bq. 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. :-/
no problem :) this code base has a long history and parts of it are confusing.
We're currently building up a shared understanding of the code in question and
you're doing a good job here.
> 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)
