cklein05 commented on pull request #428:
URL: https://github.com/apache/tomcat/pull/428#issuecomment-877095921
Okay, let's make some decisions (Bob Ross? _Here's a class, and it has a
friend..._)
Since there's no public `setFoo()` method in `GenericPrincipal` (attributes
are only set through the constructor and retrieved by `getAttribute(String
name)`), I don't see a big need for using `Collections.unmodifiableMap()`.
Again, with reflection one could access the hash map and modify entries, but
that would also be uber-paranoid (BTW, did you mean German über-paranoid?). On
the other hand, wrapping the map with an unmodifiable view will not hurt... So,
it's up to you to decide.
Both `DataSourceRealm` and `JNDIRealm` are the only components that
actually add attributes to `GenericPrincipal`´s attributes map
(`UserDatabaseRealm` and `MemoryRealm` work differently by relying on the
`User` instance). So, defensively copying attributes on input is not really
required, right? The attributes values are obtained from either JDBC or JNDI
so, no application code should have another reference to these.
With defensive copies, we could do two things:
1. Remove object duplication in `getAttribute(String name)` for all types
Pros: simple, fast
Cons: _shallow copy_ vs. _deep copy_ problem
2. Remove object duplication in `getAttribute(String name)` for known
_immutable_ types only
Pros: *no* _shallow copy_ vs. _deep copy_ problem
Cons: more complex code, slow, more CPU load on attribute retrieval, can't
tell whether a generic class is immutable
Although option 2 has much more cons, that may not be a big issue in
practice, since most of the attributes are actually Strings, Numbers and
Booleans only (e.g. Sun's standard LDAP provider returns Strings only). The
only types for which this may hurt a bit are arrays (both `DataSourceRealm` and
`JNDIRealm` may gather multi-value attributes).
With option 2, I would just modify method `GenericPrincipal.copyObject` and
leave `GenericPrincipal.copySerializableObject` untouched. The only difference
will be, that `copyObject` does a simple `return obj;` for many or most of the
_commonly object types_. So, no much code is saved with option 2 but, there are
likely no extra allocations needed for most of the attribute values returned.
So, what to do? (checked my personal choices)
- [x] use `Collections.unmodifiableMap()`
- [ ] copy values on input
- [ ] no defensive copies for _all_ types
- [x] no defensive copies for known _immutable_ types only
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
