Bug 66665: Provide option to supply role mapping from a properties file #631
Conversation
|
If there are no objections, I will merge this week. |
There was a problem hiding this comment.
I like the idea of exposing this feature. I'm somewhat surprised that it isn't part of the specification but, unless I am reading the XSDs incorrectly, the spec only defines role-mapping on a per Servlet basis which seems odd to me.
I'm not convinced an extra file configuration is necessary. This looks like something that could be a nested element in the context.xml file and implemented with an extra digester rule.
|
|
||
| private static final String WEBAPP_RESOURCE_PREFIX = "webapp:"; | ||
| private static final String CLASSPATH_RESOURCE_PREFIX = "classpath:"; | ||
|
|
There was a problem hiding this comment.
This creates a new scheme for naming configuration files. It should instead use the ConfigFileLoader and ConfigurationSource. They would need extending to include web application relative resources as they currently only support absolute file, files relative to $CATALINA_BASE, classpath and URI.
There was a problem hiding this comment.
In the specific case of this feature, it seems the only "real" location that makes sense would be the default one anyway. So I would simply remove the option to have it in random places and be done with it.
There was a problem hiding this comment.
In the specific case of this feature, it seems the only "real" location that makes sense would be the default one anyway. So I would simply remove the option to have it in random places and be done with it.
I disagree, see my explanation below.
There was a problem hiding this comment.
This creates a new scheme for naming configuration files. It should instead use the
ConfigFileLoaderandConfigurationSource. They would need extending to include web application relative resources as they currently only support absolute file, files relative to $CATALINA_BASE, classpath and URI.
Correct, that is a problem. I didn't know what features are available to make this happen. Let me look into the mentioned classes and try to rewrite it.
There was a problem hiding this comment.
This creates a new scheme for naming configuration files. It should instead use the
ConfigFileLoaderandConfigurationSource. They would need extending to include web application relative resources as they currently only support absolute file, files relative to $CATALINA_BASE, classpath and URI.
@markt-asf While looking into this, I understand how the system works how classpath: is registered with the JVM, hoping that it will use the webapp's classpath, but I fail to see how to provide the Context to org.apache.tomcat.util.file.ConfigFileLoader.getSource() without modifing it. Any pointers I could evaluate? I could of course first look into servlet context and then if not found pass to the ConfigFileLoader... (chaining basically)
There was a problem hiding this comment.
Ok. I tried to find a spot to add a new call in the configuration source for a "public Resource getResource(Context context, String name)" but IMO this doesn't add anything and also there's no ideal spot for that.
Correct, I would consider improving the ConfigurationSource a separate discussion which should not be solved here. If the class is being improved, I'd be happy to skim this class after that.
There was a problem hiding this comment.
After thinking about it for a bit, adding a helper method to the Context interface seemed like the way to go to me. This is helpful to allow more flexibility on location of configs that can be bundled in the webapp, and also it makes the ConfigurationSource API a bit more visible.
There was a problem hiding this comment.
I see what you are after:
public Resource org.apache.catalina.Context#getResource(String) which will probe for webapp: and the delegate to ConfigFileLoader?
I will happily add this, but it should be a separate PR after this one. Then when the new PR is done, I can modify this listener and it will be its first use case. Is that OK for you?
There was a problem hiding this comment.
Of course, this is a minor detail.
There was a problem hiding this comment.
Great, then I will proceed with the merge and start working on the draft.
Correct and I consider the per-Servlet one as unusable if you have tens of them.
Both should be possible because the file could be in the classpath which contains more than just the mapping. Just being in the context.xml it is not available to the actual application, but just to Tomcat. In a future revision the source could be done flexible. At least in my usecase, having it in context.xml would force me to duplicate the mapping and that is unacceptable. |
|
@rmaucher @markt-asf Incorporated your comments. Please have a look again. |
|
Merged manually. |
No description provided.