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

Configuration placement is inconsistent through the project #27

Open
ydahhrk opened this issue May 8, 2017 · 5 comments
Open

Configuration placement is inconsistent through the project #27

ydahhrk opened this issue May 8, 2017 · 5 comments
Labels
Milestone

Comments

@ydahhrk
Copy link
Member

ydahhrk commented May 8, 2017

This documentation from "behavior-configuration" seems to be incorrect:

The WEB-INF folder is the default location of the configuration.properties file, but you can define a new location by following these steps:

  1. In WEB-INF/web.xml, find the following lines:
  <!-- <param-name> -->
  	rdapConfigurationUserPath
  <!-- </param-name> -->
  <!-- <param-value> -->
  <!-- </param-value> -->
  1. Uncomment them, and write a valid directory path in param-value.
  2. Save the changes and test the configuration.

First, because the "rdapConfigurationUserPath" is not commented, this doesn't seem to be valid XML. Secondly, I could not find this code in the predefined web.xml so there was nothing to uncomment. Third, it seems that it was envisioned that the user would be able to enter an absolute path to whatever file anywhere, but this is not working: The code never actually queries the filesystem outside of the classpath and the servlet context.

So this is currently a seemingly pointless feature; one can redefine the configuration file, but only if it still is within the context.

response-privacy largely suffers from the same problems.

I think that this was the original vision for all the configuration:

  1. First, query the classpath (ClassLoader#getResourceAsStream(String)) to get default values.
  2. Then, if the user somehow defined custom locations for configuration files, then try and find that in the servlet context (ServletContext#getResourceAsStream(String)), and if that fails, try and fall back to find them elsewhere (Files#newInputStream(Path)).
  3. Otherwise (if there were no custom file definitions) find the files in their default servlet context locations (Again, ServletContext#getResourceAsStream(String)).

On the other hand, from reading the code, it seems like the situation is as follows:

  • In general, servlet configuration has classpath default values (1 ok) and normally resides in the servlet context (3 ok). If the user defined custom locations, they only work if these locations are still within the context (2 bad).
  • Basic data access implementation configuration lacks default values (1 ok), and normally resides in the servlet context (as the server reads it and passes it along to the implementation), though if more than a single properties file is needed, the implementation needs to use the classpath instead because it doesn't know whether it is serving a servlet application (hence, the servlet context might not exist) (2 and 3 more or less bad).
  • The configuration for the help response specifically (the XMLs) is the only one where the entire original vision appears to be followed correctly.

In other words, configuration placement is a bit of a mess right now. We need to first agree on a standard and then follow it.

@dhfelix
Copy link
Contributor

dhfelix commented May 8, 2017

This documentation from "behavior-configuration" seems to be incorrect:

The WEB-INF folder is the default location of the configuration.properties file, but you can define a new location by following these steps:

In WEB-INF/web.xml, find the following lines:

  rdapConfigurationUserPath

Uncomment them, and write a valid directory path in param-value.
Save the changes and test the configuration.
First, because the "rdapConfigurationUserPath" is not commented, this doesn't seem to be valid XML. Secondly, I could not find this code in the predefined web.xml so there was nothing to uncomment.

I think the documentation should say, "If you want to define a different path for the configuration files add the parameter" rdapConfigurationUserPath "in the web.xml file" and then put an example of how to put the parameter in the xml file or simply Add in the xml file the parameters to be configured, disabled correctly, to be consistent with the documentation.

Third, it seems that it was envisioned that the user would be able to enter an absolute path to whatever file anywhere, but this is not working: The code never actually queries the filesystem outside of the classpath and the servlet context.

So this is currently a seemingly pointless feature; one can redefine the configuration file, but only if it still is within the context.

response-privacy largely suffers from the same problems.

I think that this was the original vision for all the configuration:

First, query the classpath (ClassLoader#getResourceAsStream(String)) to get default values.
Then, if the user somehow defined custom locations for configuration files, then try and find that in the servlet context (ServletContext#getResourceAsStream(String)), and if that fails, try and fall back to find them elsewhere (Files#newInputStream(Path)).
Otherwise (if there were no custom file definitions) find the files in their default servlet context locations (Again, ServletContext#getResourceAsStream(String)).

My opinion on how to read the settings is as follows.

  • First read the default configuration provided.

  • Second, if the user configures a path to a directory, the rdap-server will read the configuration to that path, the path should be provided as an absolute path.

  • If the user does not configure a path to a directory, rdap-server should read the configuration in the default location defined in the documentation (usually WEB-INF).

Keep in mind that if the user sets a path to a directory to read the configuration, the configuration file should no longer be read in the default directory that the documentation indicates. (This is a personal comment, but we can always do that if there is no configuration in the path defined by the user, the rdap-server looks in the default folder defined in the documentation.)

The configuration provided by the user will overwrite the values ​​of the default configuration. I.e. If the user only overwrites a single parameter, the other parameters will retain their default values.

In other words, configuration placement is a bit of a mess right now. We need to first agree on a standard and then follow it.

I Agree

@DAlpuche
Copy link
Contributor

DAlpuche commented May 8, 2017

The WEB-INF folder is the default location of the configuration.properties file, but you can define a new location by following these steps:

In WEB-INF/web.xml, find the following lines:

rdapConfigurationUserPath

Uncomment them, and write a valid directory path in param-value.
Save the changes and test the configuration.

There is no doubt that the documentation above is a mess. So, will the user really use that feature? I think this can be kind of confussing.

I think that this was the original vision for all the configuration:

1.First, query the classpath (ClassLoader#getResourceAsStream(String)) to get default values.
2.Then, if the user somehow defined custom locations for configuration files, then try and find that in the servlet context (ServletContext#getResourceAsStream(String)), and if that fails, try and fall back to find them elsewhere (Files#newInputStream(Path)).
3.Otherwise (if there were no custom file definitions) find the files in their default servlet context locations (Again, ServletContext#getResourceAsStream(String)).

My opinion is that the behavior should only consist of point 1 and 3 (together), in other words, load the default configuration, and then, rdap-server should read the configuration in the default location defined in the documentation (WEB-INF) looking for overwrited parameters, as @dhfelix said:

The configuration provided by the user will overwrite the values ​​of the default configuration. I.e. If the user only overwrites a single parameter, the other parameters will retain their default values.

I think we should only give the users the option to write their configuration in the folder WEB-INF.

@ydahhrk
Copy link
Member Author

ydahhrk commented May 9, 2017

(DAlpuche)
So, will the user really use that feature? I think this can be kind of confussing.
(...)
My opinion is that the behavior should only consist of point 1 and 3 (together)

Really? I was under the impression that you were the one who wanted this feature. Then who was? What was the point?

(dhfelix)
Second, if the user configures a path to a directory, the rdap-server will read the configuration to that path, the path should be provided as an absolute path.

Yeah, I kind of feel that there is something inherently wrong about mixing the servlet context directory structure and the actual filesystem directory structure, which is what the help loading code is doing by querying the servlet context first and the filesystem second, using the same string.

Perhaps an alternate solution would be to define two separate web.xml keys:

<context-param>
	<param-name> rdapConfigFileAbsolute </param-name>
	<param-value> /home/myself/Desktop/config.properties </param-value>
</context-param>

<context-param>
	<param-name> rdapConfigFileContext </param-name>
	<param-value> WEB-INF/config.properties </param-value>
</context-param>

And the user would use whichever feels right and ignore the other.

But perhaps there is no point in doing this at all. It all comes down to why we wanted to support absolute paths in the first place. Do we have an answer for that?

@ydahhrk
Copy link
Member Author

ydahhrk commented May 9, 2017

(dhfelix)
Keep in mind that if the user sets a path to a directory to read the configuration, the configuration file should no longer be read in the default directory that the documentation indicates.

Agree.

(dhfelix)
(This is a personal comment, but we can always do that if there is no configuration in the path defined by the user, the rdap-server looks in the default folder defined in the documentation.)

This will lead to confusion; we don't want to accidentally load values the user is explicitly telling us not to.

@pcarana pcarana added this to the v1.1.3 milestone Sep 22, 2017
@dhfelix
Copy link
Contributor

dhfelix commented Oct 13, 2017

Talking with @pcarana, we conclude that the way the application configuration should be read is as follows:

  • First read the default configuration provided.
  • Second, if the user configures an absolute path to a directory outside of the context of the web application, the rdap-server will read the configuration to that path, the path should be provided as an absolute path.
  • If the user does not configure a path to a directory, rdap-server should read the configuration in the default location defined in the documentation (usually WEB-INF).

Users will not be able to configure where the application can read the configuration inside the rdap-server.

@dhfelix dhfelix added the Solved label Oct 13, 2017
dhfelix pushed a commit that referenced this issue Apr 12, 2018
Includes removing the lies detected in #27.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants