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

[ELY-806] missing Permission parameters handling #592

Closed
wants to merge 1 commit into from
Closed

[ELY-806] missing Permission parameters handling #592

wants to merge 1 commit into from

Conversation

hkalina
Copy link

@hkalina hkalina commented Dec 2, 2016

@@ -435,13 +435,19 @@ public static Permission createPermission(final Class<? extends Permission> perm
}
}
try {
if (twoArg != null) {
if (twoArg != null && name != null && actions != null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Normally it's OK to call a permission constructor with no name or actions, and sometimes the two-arg constructor is the only one available even if the permission type ignores the arguments.

Is there a failure case here?

Copy link
Author

Choose a reason for hiding this comment

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

Well that is problem... in such case I will not obtain info that the problem is in missing argument (or which one?)...

Copy link
Contributor

Choose a reason for hiding this comment

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

Only the permission implementation class can know what arguments are required or what their constraints are, unfortunately.

Copy link
Author

Choose a reason for hiding this comment

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

Can we rely that IllegalArgumentException is thrown at least?
(If not, I think it will be better to close this issue without fix as not fixable then exception hacking...)

Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately no. Some classes may throw NPE - deliberately or accidentially.

*
* @author <a href="mailto:jkalina@redhat.com">Jan Kalina</a>
*/
public class InvalidNamedArgumentException extends IllegalArgumentException {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like this too much. I'd prefer to rely on localized error messages instead, unless there's something that can be done about the exception by automated code.

Copy link
Author

Choose a reason for hiding this comment

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

The reason is automatic rewriting of permission constructor argument name (like "name") to subsystem resource attribute ("target-name") when catching and rethrowing in subsystem.

Copy link
Contributor

@dmlloyd dmlloyd left a comment

Choose a reason for hiding this comment

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

Changes requested as previously described.

@hkalina hkalina closed this Dec 8, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants