Add support for collections and iterables to authZ generators #761
Conversation
|
+1 |
| private final String[] permissionAttributes; | ||
|
|
||
|
|
||
| private final Iterable<String> roleAttributes; |
There was a problem hiding this comment.
Maybe we could have Collection everywhere, as in the other class? I'm not so clear about the difference between using Iterable or Collection in this context... I feel like Collection is the main way to represent a collection of something, Iterable seems too low level in a way. Not sure :)
| return Arrays.copyOf(original, original.length); | ||
| public FromAttributesAuthorizationGenerator(final String[] roleAttributes, final String[] permissionAttributes) { | ||
| if (roleAttributes != null) { | ||
| this.roleAttributes = Arrays.asList(roleAttributes); |
There was a problem hiding this comment.
Careful, the behaviour is a little changed: Arrays.copyOf was making a copy, while Arrays.asList use the array itself as the backend of an ArrayList; changes to the array will be reflected in the ArrayList.
There was a problem hiding this comment.
What sort of changes to the array do you have in mind? Could you demonstrate that with an example perhaps?
There was a problem hiding this comment.
I'm thinking of this kind or that kind of best practice.
Basically in object oriented programming, the object encapsulates data and if this data is directly modifiable by an external object, then you have a problem. So when you can, you should copy mutable data in prevention, in particular when you don't have responsibility of the code that will call the concerned method. It's a very basic best practice and it was already enforced in the previous code you modify here, so I thought it would be best to continue with that.
There was a problem hiding this comment.
I can't think of a practical scenario in this case where that would make sense. If you insist, I can make a change, sure, but I don't see how this applies really.
There was a problem hiding this comment.
do as you wish, sorry for the bother :)
There was a problem hiding this comment.
Oh no worries at all. Thanks. I understand the principal and I don't mind it at all; mostly wanted to see if there was anything concrete that I should pay closer attention to. Updates coming up.
Closes #758