[
https://issues.apache.org/jira/browse/OAK-4257?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15252092#comment-15252092
]
angela edited comment on OAK-4257 at 4/21/16 4:06 PM:
------------------------------------------------------
[~alfu], I had a look at the patch. As a general note it would probably better
if you would group patches for different modules and create individual issues
for them; this would allow those committers to look at the patch that is
familiar with that particular code base.
Here my summary from the patch:
- test-cases in {{oak-auth-external}}: i merged some of the additional tests
for {{null}}; others don't make too much sense to me as they can be considered
setup calls (i.e. retrieving an external user from the IDP, which we control),
which IMHO don't need to be checked for null. that makes the test harder to
read (IMHO the assertions should actually verify the test results) and any
unexpected NPE would fail the test, which would be fine.
- {{CugPolicyImplTest}}: it's just an unused variable; I don't think this needs
to be fixed, specially in a test-case.
- {{AccessControlValidator}}: I would rather consider these false positive as
the mentioned properties are mandatory by the underlying node type definition
(-> checked in another validator)... if we guard against {{null}} this should
result in an {{CommitFailedException}} and not silently proceeding with some
default.
- {{AuthorizationContext}}: that's indeed a potential NPE; but i would rather
test for {{null}} before calling the {{isNtName}} method instead of removing
the non-null-annotation => will fix.
- {{CompositeAuthorizationConfiguration}}: that really looks like a bug... I
will also create a test-case to verify.
Then for the other stuff:
- {{S3Backend}}: I think this is actually a bug in the flow because the code
tests for a {{null}} callback in the try, throws {{IllegalArgumentException}},
catches {{Exception}} and calls abort on the {{callback}}. May I kindly ask you
to create a separate, dedicated issue for this in the corresponding component
such that [~amitjain] can take a look? thanks.
- {{RemoteValue}}: as far as I can see this is a false positive; but: the
javadoc seems to have a copy-paste error, which makes this a bit confusing. May
I ask you to create a separate, dedicated issue for that and have it clarified
by [~frm]? thanks.
- {{MigrationOptions}}: same here. please create a dedicated, separate issue
explaining your findings as I don't feel familiar enough with the code. thanks.
was (Author: anchela):
[~alfu], I had a look at the patch. As a general note it would probably better
if you would group patches for different modules and create individual issues
for them; this would allow those committers to look at the patch that is
familiar with that particular code base.
Here my summary from the patch:
- test-cases in {{oak-auth-external}}: i merged some of the additional tests
for {{null}}; others don't make too much sense to me as they can be considered
setup calls (i.e. retrieving an external user from the IDP, which we control),
which IMHO don't need to be checked for null. that makes the test harder to
read (IMHO the assertions should actually verify the test results) and any
unexpected NPE would fail the test, which would be fine.
- {{CugPolicyImplTest}}: it's just an unused variable; I don't think this needs
to be fixed, specially in a test-case.
- {{AccessControlValidator}}: I would rather consider these false positive as
the mentioned properties are mandatory by the underlying node type
definition... if we guard against {{null}} this should result in an
{{CommitFailedException}} and not silently proceeding with some default.
- {{AuthorizationContext}}: that's indeed a potential NPE; but i would rather
test for {{null}} before calling the {{isNtName}} method instead of removing
the non-null-annotation => will fix.
- {{CompositeAuthorizationConfiguration}}: that really looks like a bug... I
will also create a test-case to verify.
Then for the other stuff:
- {{S3Backend}}: I think this is actually a bug in the flow because the code
tests for a {{null}} callback in the try, throws {{IllegalArgumentException}},
catches {{Exception}} and calls abort on the {{callback}}. May I kindly ask you
to create a separate, dedicated issue for this in the corresponding component
such that [~amitjain] can take a look? thanks.
- {{RemoteValue}}: as far as I can see this is a false positive; but: the
javadoc seems to have a copy-paste error, which makes this a bit confusing. May
I ask you to create a separate, dedicated issue for that and have it clarified
by [~frm]? thanks.
- {{MigrationOptions}}: same here. please create a dedicated, separate issue
explaining your findings as I don't feel familiar enough with the code. thanks.
> Findbug issues (mostly NPEs)
> ----------------------------
>
> Key: OAK-4257
> URL: https://issues.apache.org/jira/browse/OAK-4257
> Project: Jackrabbit Oak
> Issue Type: Technical task
> Components: auth-external, authorization-cug, remoting, security,
> upgrade
> Reporter: Alfusainey Jallow
> Priority: Trivial
> Attachments: findbugs-trivial-issues.patch
>
>
> Sub task addressing trivial issues reported by the findbugs static analyzer.
> the issues addressed are mostly NPEs plus a few in oak-security
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
