[TOMEE-2060] Make app naming context read only (switched on/off with property) #72
Conversation
There was a problem hiding this comment.
Good start, few "style" changes for the main PR (mainly to align on other properties style and ensure the change can be seen by the user if it breaks something)
Also would be great to test it, we already have some IvmContextTests so should be enriched to test all these changes. Also please add a test in tomee (you can use tomee embedded or arquillian tests module) since here the context federation behaves differently.
Once these changes are covered we can integrate it to the code source
|
|
||
| //TODO: set proper default | ||
| //required by spec: EE.5.3.4, used together with tomcat#jndiExceptionOnFailedWrite | ||
| if("true".equals(SystemInstance.get().getProperty("forceReadOnlyAppNamingContext", "true"))) { |
There was a problem hiding this comment.
please prefix it openejb. (openejb.forceReadOnlyAppNamingContext)
also shouldnt the default be false to ensure it is writable by default (backward compatibility)
also think createApplication.success is not the right key yet ;)
| } else if(ctx instanceof ContextHandler) { | ||
| ((ContextHandler)ctx).setReadOnly(); | ||
| } else { | ||
| //TODO: log only? |
There was a problem hiding this comment.
shouldn't be possible so can be ignored for now
| if(ctx instanceof IvmContext) { | ||
| ((IvmContext) ctx).setReadOnly(true); | ||
| } else if(ctx instanceof ContextHandler) { | ||
| ((ContextHandler)ctx).setReadOnly(); |
There was a problem hiding this comment.
side note on casting: on the list we got a discussion saying we prefer the ${type}.class.cast(instance) style instead of (($type) instance). Not a blocker for me but just mentionning it if you see that style in the code elsewhere.
| checkReadOnly(); | ||
| if(checkReadOnly()) { | ||
| //TODO: throw exception or return null? tomcat returns null | ||
| return null; |
There was a problem hiding this comment.
null is fine if there is a one time - 10 calls will log a single time - log line (warning?)
| if (readOnly) { | ||
| throw new OperationNotSupportedException(); | ||
| //alignment with tomcat behavior | ||
| if("true".equals(System.getProperty("jndiExceptionOnFailedWrite"))) { |
There was a problem hiding this comment.
SystemInstance.get().getProperty and openejb prefix as well, default should fail as before
| } | ||
|
|
||
| public void setReadOnly(boolean isReadOnly) { | ||
| //TODO: should it log? |
There was a problem hiding this comment.
no need to log it until there is an issue with this being called (which is more in write methods)
|
applied, build in progress on https://ci.apache.org/builders/tomee-trunk-ubuntu/builds/778 |
|
Thank you for reviewing this. Three more questions/comments from my side:
|
|
|
@katya-stoycheva can you please close this - it was applied. Thank you very much for your contribution :-) |
updates tomcat version
Initial proposal (there are things to be clarified)