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

[TOMEE-2060] Make app naming context read only (switched on/off with property) #72

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -81,6 +81,7 @@
import org.apache.openejb.core.SimpleTransactionSynchronizationRegistry;
import org.apache.openejb.core.TransactionSynchronizationRegistryWrapper;
import org.apache.openejb.core.WebContext;
import org.apache.openejb.core.ivm.ContextHandler;
import org.apache.openejb.core.ivm.IntraVmProxy;
import org.apache.openejb.core.ivm.naming.ContextualJndiReference;
import org.apache.openejb.core.ivm.naming.IvmContext;
Expand Down Expand Up @@ -1029,6 +1030,14 @@ private AppContext createApplication(final AppInfo appInfo, ClassLoader classLoa

systemInstance.fireEvent(new AssemblerAfterApplicationCreated(appInfo, appContext, allDeployments));
logger.info("createApplication.success", appInfo.path);

//TODO: set proper default
//required by spec: EE.5.3.4, used together with tomcat#jndiExceptionOnFailedWrite
if("true".equals(SystemInstance.get().getProperty("forceReadOnlyAppNamingContext", "true"))) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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 ;)

ensureAppNamingContextIsReadOnly(allDeployments);
//TODO message
logger.info("createApplication.success", appInfo.path);
}

return appContext;
} catch (final ValidationException | DeploymentException ve) {
Expand All @@ -1049,6 +1058,19 @@ private AppContext createApplication(final AppInfo appInfo, ClassLoader classLoa
}
}

private void ensureAppNamingContextIsReadOnly(final List<BeanContext> allDeployments) {
for(BeanContext beanContext : allDeployments) {
Context ctx = beanContext.getJndiContext();
if(ctx instanceof IvmContext) {
((IvmContext) ctx).setReadOnly(true);
} else if(ctx instanceof ContextHandler) {
((ContextHandler)ctx).setReadOnly();
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

} else {
//TODO: log only?
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't be possible so can be ignored for now

}
}
}

private List<String> getDuplicates(final AppInfo appInfo) {
final List<String> used = new ArrayList<String>();
for (final EjbJarInfo ejbJarInfo : appInfo.ejbJars) {
Expand Down Expand Up @@ -2143,6 +2165,9 @@ public void destroyApplication(final AppInfo appInfo) throws UndeployException {
continue;
}

if(globalContext instanceof IvmContext) {
((IvmContext) globalContext).setReadOnly(false);
}
unbind(globalContext, path);
unbind(globalContext, "openejb/global/" + path.substring("java:".length()));
unbind(globalContext, path.substring("java:global".length()));
Expand Down
Expand Up @@ -19,6 +19,7 @@

import org.apache.openejb.core.ThreadContext;
import org.apache.openejb.core.ivm.naming.ContextWrapper;
import org.apache.openejb.core.ivm.naming.IvmContext;
import org.apache.openejb.loader.SystemInstance;
import org.apache.openejb.spi.ContainerSystem;

Expand Down Expand Up @@ -97,4 +98,10 @@ public Object lookup(final String name) throws NamingException {
throw nnfe;
}
}

public void setReadOnly() {
if(this.context instanceof IvmContext) {
((IvmContext) context).setReadOnly(true);
}
}
}
Expand Up @@ -87,7 +87,12 @@ public IvmContext(final String nodeName) {
}

public IvmContext(final NameNode node) {
this(node, false);
}

public IvmContext(final NameNode node, final boolean isReadOnly) {
mynode = node;
readOnly = isReadOnly;
// mynode.setMyContext(this);
}

Expand Down Expand Up @@ -147,7 +152,7 @@ If the object has been resolved in the past from this context and the specified
Object obj = fastCache.get(compoundName);
if (obj == null) {
try {
obj = mynode.resolve(new ParsedName(compoundName));
obj = mynode.resolve(new ParsedName(compoundName), readOnly);
} catch (final NameNotFoundException nnfe) {
obj = federate(compositName);
}
Expand Down Expand Up @@ -293,7 +298,9 @@ public Object lookup(final Name compositName) throws NamingException {
}

public void bind(String name, final Object obj) throws NamingException {
checkReadOnly();
if(checkReadOnly()) {
return;
}
final int indx = name.indexOf(":");
if (indx > -1) {
/*
Expand Down Expand Up @@ -328,7 +335,9 @@ public void rebind(final Name name, final Object obj) throws NamingException {
}

public void unbind(String name) throws NamingException {
checkReadOnly();
if(checkReadOnly()) {
return;
}
final int indx = name.indexOf(":");
if (indx > -1) {
/*
Expand Down Expand Up @@ -399,7 +408,10 @@ public void destroySubcontext(final Name name) throws NamingException {
}

public Context createSubcontext(String name) throws NamingException {
checkReadOnly();
if(checkReadOnly()) {
//TODO: throw exception or return null? tomcat returns null
return null;
Copy link
Contributor

Choose a reason for hiding this comment

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

null is fine if there is a one time - 10 calls will log a single time - log line (warning?)

}
final int indx = name.indexOf(":");
if (indx > -1) {
/*
Expand All @@ -411,7 +423,7 @@ public Context createSubcontext(String name) throws NamingException {
if (fastCache.containsKey(name)) {
throw new NameAlreadyBoundException();
} else {
return mynode.createSubcontext(new ParsedName(name));
return mynode.createSubcontext(new ParsedName(name), readOnly);
}
}

Expand Down Expand Up @@ -477,9 +489,36 @@ public String getNameInNamespace() throws NamingException {
public void close() throws NamingException {
}

protected void checkReadOnly() throws OperationNotSupportedException {
//TODO: rename? isReadOnly?
/*
* return false if current naming context is not marked as read only
* return true if current naming context is marked as read only and system property jndiExceptionOnFailedWrite is set to false
*
* throws OperationNotSupportedException if naming context:
* - is marked as read only and
* - system property jndiExceptionOnFailedWrite is set to true
*
* jndiExceptionOnFailedWrite property is defined by tomcat and is used in similar context for web app components
* https://tomcat.apache.org/tomcat-7.0-doc/config/context.html#jndiExceptionOnFailedWrite
*
*/
protected boolean checkReadOnly() throws OperationNotSupportedException {
//TODO: should it log?
if (readOnly) {
throw new OperationNotSupportedException();
//alignment with tomcat behavior
if("true".equals(System.getProperty("jndiExceptionOnFailedWrite"))) {
Copy link
Contributor

Choose a reason for hiding this comment

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

SystemInstance.get().getProperty and openejb prefix as well, default should fail as before

throw new OperationNotSupportedException();
}
return true;
}
return false;
}

public void setReadOnly(boolean isReadOnly) {
//TODO: should it log?
Copy link
Contributor

Choose a reason for hiding this comment

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

no need to log it until there is an issue with this being called (which is more in write methods)

this.readOnly = isReadOnly;
if(mynode != null) {
mynode.setReadOnly(readOnly);
}
}

Expand Down
Expand Up @@ -26,6 +26,7 @@
import java.io.PrintStream;
import java.io.Serializable;
import java.util.ArrayList;
import java.util.Iterator;

public class NameNode implements Serializable {
private final String atomicName;
Expand Down Expand Up @@ -59,12 +60,17 @@ void setMyContext(final IvmContext myContext) {
this.myContext = myContext;
}

//TODO: probably can be removed, doesn't seem to be used anywhere
public Object getBinding() {
return getBinding(false);
}

public Object getBinding(boolean createReadOnlyContext) {
if (myObject != null && !(myObject instanceof Federation)) {
return myObject;// if NameNode has an object it must be a binding
} else {
if (myContext == null) {
myContext = new IvmContext(this);
myContext = new IvmContext(this, createReadOnlyContext);
}
return myContext;
}
Expand All @@ -73,8 +79,13 @@ public Object getBinding() {
public Object getObject() {
return myObject;
}


//TODO: probably can be removed, doesn't seem to be used anywhere
public Object resolve(final ParsedName name) throws NameNotFoundException {
return resolve(name, false);
}

public Object resolve(final ParsedName name, boolean createReadOnlyContext) throws NameNotFoundException {
final int compareResult = name.compareTo(atomicHash);
NameNotFoundException n = null;
final int pos = name.getPos();
Expand All @@ -83,30 +94,30 @@ public Object resolve(final ParsedName name) throws NameNotFoundException {
if (name.next()) {
if (subTree != null) {
try {
return subTree.resolve(name);
return subTree.resolve(name, createReadOnlyContext);
} catch (final NameNotFoundException e) {
n = e;
}
} else if (!subTreeUnbound && !unbound && myContext != null && !Federation.class.isInstance(myObject)) {
try {
return myContext.mynode.resolve(name);
return myContext.mynode.resolve(name, createReadOnlyContext);
} catch (final NameNotFoundException e) {
n = e;
}
}
} else if (!unbound) {
return getBinding();
return getBinding(createReadOnlyContext);
}
} else if (compareResult == ParsedName.IS_LESS) {
// parsed hash is less than
if (lessTree != null) {
return lessTree.resolve(name);
return lessTree.resolve(name, createReadOnlyContext);
}

} else {
//ParsedName.IS_GREATER
if (grtrTree != null) {
return grtrTree.resolve(name);
return grtrTree.resolve(name,createReadOnlyContext);
}
}
if (myObject instanceof Federation) {
Expand All @@ -130,7 +141,7 @@ public Object resolve(final ParsedName name) throws NameNotFoundException {
}
if (f != null) {
final NameNode node = new NameNode(null, new ParsedName(""), f, null);
return new IvmContext(node);
return new IvmContext(node, createReadOnlyContext);
}
}
if (n != null) {
Expand Down Expand Up @@ -292,6 +303,32 @@ private void unbind(final NameNode node) {
}
rebalance(node);
}

void setReadOnly(boolean isReadOnly) {
if(myContext != null) {
myContext.readOnly = isReadOnly;
}

if(myObject instanceof Federation) {
Iterator<Context> federatedContextsIterator = ((Federation) myObject).iterator();
while(federatedContextsIterator.hasNext()) {
Context current = federatedContextsIterator.next();
//TODO: what other types of federated contexts than IvmContext?
if(current instanceof IvmContext) {
((IvmContext)current).setReadOnly(isReadOnly);
}
}
}
if (subTree != null) {
subTree.setReadOnly(isReadOnly);;
}
if (lessTree != null) {
lessTree.setReadOnly(isReadOnly);
}
if (grtrTree != null) {
grtrTree.setReadOnly(isReadOnly);
}
}

private void rebalance(final NameNode node) {
if (node.subTree != null) {
Expand Down Expand Up @@ -361,12 +398,17 @@ protected void clearCache() {
subTree.clearCache();
}
}


//TODO: probably can be removed, doesn't seem to be used anywhere
public IvmContext createSubcontext(final ParsedName name) throws NameAlreadyBoundException {
return createSubcontext(name, false);
}

public IvmContext createSubcontext(final ParsedName name, final boolean createReadOnlyContext) throws NameAlreadyBoundException {
try {
bind(name, null);
name.reset();
return (IvmContext) resolve(name);
return (IvmContext) resolve(name, createReadOnlyContext);
} catch (final NameNotFoundException exception) {
exception.printStackTrace();
throw new OpenEJBRuntimeException(exception);
Expand Down