Configuration troubles

[Migrated From Jforum.net]

I've got the cvs version running (with the com.sysdeo.eclipse.tomcat_3.0.0.alpha1 plugin).
The code is looking great! (apart from some comments in a language I cannot read :lol:)

I did encounter some configuration problems, maybe you can shed some light on these problems and possible structural solutions:
[list]
[*] Initially, the init-param 'development' was set to true in web.xml. This resulted in a NullPointer exception when the JForum index page was loaded. The problem is that when development (and thus JForum.debug) is set to true, SystemGlobals are not loaded in the servlet init function, presumably because they will be loaded on each request. But in JForum.service the following code can be found:

public void service(HttpServletRequest req, HttpServletResponse response) throws IOException, ServletException
{
    try {
        String encoding = (String)SystemGlobals.getValue("encoding");
        req.setCharacterEncoding(encoding);

        // Initializes thread local data
        DataHolder dataHolder = new DataHolder();
        localData.set(dataHolder);

        if (JForum.debug) {
            SystemGlobals.loadDefaults(SystemGlobals.getApplicationResourceDir() +"config/SystemGlobals.properties");
            SystemGlobals.loadQueries(SystemGlobals.getApplicationResourceDir() +"config/"+ SystemGlobals.getValue("sql.file"));
            this.loadUrlPatterns();
            I18n.load();
            BBCodeRepository.setBBCollection(new BBCodeHandler().parse());
        }

SystemGlobals is used before the defaults are loaded, resulting in a NullPointerException in
req.setCharacterEncoding. Is it safe to just move the character encoding stuff behind the SystemGlobals initialization? (I just set development to false for the time being)
[*] sql.file is set to mysql.sql in SystemGlobals.properties, but mysql.sql is located in conf/database/mysql/mysql.sql. This file was only found when I set sql.file to database/mysql/mysql.sql?
[/list]

[originally posted on jforum.net by Pieter]

[Migrated From Jforum.net]

Am I doing something wrong with the

[*] List stuff?

[originally posted on jforum.net by Pieter]

[Migrated From Jforum.net]

Yep, the current list tag implementation is kinda wrong. You can use [ list ] item [/ list ] for bulleted item for now.
[originally posted on jforum.net by Daniil]

[Migrated From Jforum.net]

Pieter wrote:(apart from some comments in a language I cannot read :lol:)

hehehe.. sorry.. :) .. I'll fix that.

On the rest of your post, the CVS head is quite broken, as you saw. I reorganized the directory structure for databases, and renamed the mysql driver do "generic', and didn't updated the cvs with the new SystemGlobals.properties.
The encoding stuff is the same situation.. I changed some encoding code but didn't tested it before sending to cvs, since was late and I gone sleep :).

Please try updating from cvs again tomorow ( today is thursday ).

Rafael
[originally posted on jforum.net by Rafael Steil]

[Migrated From Jforum.net]

Hello Rafael,

I've updated to your latest commit and the configuration works fine now.

I have a suggestion for the configuration though. Hardcoding everything in SystemGlobals.properties is a problem when you work with multiple developers. Unless everyone has exactly the same configuration (not likely), each developer needs his/her own version of this file. This can be a hassle as you have to make sure you do not commit over someone else's version.

I have some experience in other projects where this problem crept up. A very usable solution is to not only load the basic configuration properties (SystemGlobals.properties in this case), but in addition merge in a developer specific file if present. Entries in this file would override enties in the default file with the same key. I usually base the name of this file on the name of the current user (user.name) in combination with the hostname to avoid clashes between multiple developers with the same loginname.

This developer specific file can either be committed or put in cvsignore depending whether it contains sensitive information (database passwords etc.)

If you approve of this change I hereby volunteer to implement it, as this seems like a
good way to get more familiar with the JForum code before I tackle more difficult issues including generic support for a coupling with an external user database.

Regards,
Pieter.

[originally posted on jforum.net by Pieter]

[Migrated From Jforum.net]

Ok, you can do that .

I've created it on JIRA. The link is http://www.jforum.net/jira/browse/JF-13

Please make an account there so I can assign the issue to you.

Thanks,
Rafael
[originally posted on jforum.net by Rafael Steil]

[Migrated From Jforum.net]

Done. My login on jira is "pieter".

[originally posted on jforum.net by Pieter]

[Migrated From Jforum.net]

Just a quick question: If I read the code correctly, in non-development mode changed configuration options are only activated after you restart the jforum web application. Do you want to keep this behaviour, or do you want to effectuate changes immediately?
[originally posted on jforum.net by Pieter]

[Migrated From Jforum.net]

Pieter wrote:Just a quick question: If I read the code correctly, in non-development mode changed configuration options are only activated after you restart the jforum web application. Do you want to keep this behaviour, or do you want to effectuate changes immediately?

Well, we should not check for changes every time when in production. An option is to add a section to the admin panel to reload the config files, like queries and bb-code.

Rafael
[originally posted on jforum.net by Rafael Steil]

[Migrated From Jforum.net]

Pieter wrote:Done. My login on jira is "pieter".

Nice. I assigned jf-13 to you

Rafael
[originally posted on jforum.net by Rafael Steil]

[Migrated From Jforum.net]

I agree with you that we don't want to check config file modification at each http request.

But we don't need to reload the config files when the change took place from within the JForum admin section: we have all the information to just update the current configuration in memory (and of course also on disk) when the admin user hits the "update" button!
[originally posted on jforum.net by Pieter]

[Migrated From Jforum.net]

Pieter wrote:
But we don't need to reload the config files when the change took place from within the JForum admin section: we have all the information to just update the current configuration in memory (and of course also on disk) when the admin user hits the "update" button!

Well, what we have to update:

:arrow: The database files ( queries, config etc )
:arrow: bb_config.xml
:arrow: permissions.xml
:arrow: modulesMapping and urlPattern. But there are very very unlike to happen.

:arrow: SystemGlobals partially, since most of relevant features are available for online change at "Config" section.

Now I'm assuming that the only reason to reload these files - excluding SystemGlobals - is when, after the site is up and running, some misconfiguration is realized, or when some little bug is found ( like some invalid or incorrect bbcode's regexp ), and a context reload is not possible in a short time.

So, we should reload the changes from disk ( and not to disk ), which will bring the new stuff to the memory.

Is that your point? or I got something wrong?

Rafael
[originally posted on jforum.net by Rafael Steil]

[Migrated From Jforum.net]

An interesting approach, I don't know that book but I'm always interested to learn so can you give me the rationale behind this?

If you choose to use an abstract class instead of an interface you cannot implement the interface to use the constants unqualified.

[originally posted on jforum.net by Pieter]

[Migrated From Jforum.net]

The point I tried to make only concerned the entries in SystemGlobals.properties that can be changed in the "Config" section.

As I read the code now, changes to these values are only written to disk, not used directly. So when the admin changes these settings via the "Config" section, the webapp still needs reloading for the changes to be activated. It seems that this is not what you want: when the admin changes something the change(s) should be visible immediately.

I believe it is quite allright that any changes to the other config files you mention (or settings in SystemGlobals.properties that are not editable from within the "Config" section) require a reload of the webapp to be activated.

If we decide later on that there are any other settings that change frequently enough to warrant "hot pluggable" changes, we should probably make them editable from some admin section.

[originally posted on jforum.net by Pieter]

[Migrated From Jforum.net]

Pieter wrote:
As I read the code now, changes to these values are only written to disk, not used directly. So when the admin changes these settings via the "Config" section, the webapp still needs reloading for the changes to be activated.

ARGH!

Ok, this is an ugly bug. :? . Indeed, the values in memory are not updated. I created an entry on the bug tracker:

http://www.jforum.net/jira/browse/JF-15

It is assigned to you . If you could not do it ( or don't want ), please tell me so then I change the assignee.

Rafael
[originally posted on jforum.net by Rafael Steil]

[Migrated From Jforum.net]

No problem, this will be fixed as a "side effect" of the user specific preference implementation.

On another note, I'd like to change all hardcoded configuration keys into constants if you don't mind? The constants will be gathered into a single interface ConfigKeys.

For instance ConfigKeys will contain:
static final String FORUM_LINK = "forum.link";
and I will replace all literal uses of "forum.link" with ConfigKeys.FORUM_LINK

This will give us the following benefits:
- More typesafety (the compiler will warn if you mistype the name of a config key)
- A nice overview of all configuration keys (although SystemGlobals.properties already serves for this purpose) with the possibility to comment on each
- This lets us autocomplete configuration keys under eclipse ;-)


[originally posted on jforum.net by Pieter]

[Migrated From Jforum.net]

Nice, go ahead! The JIRA's entry is http://www.jforum.net/jira/browse/JF-16. When you start the job, please click in the link "Start Progress", and "Resolve Issue" when you finish it.

One last thing: Please make ConfigKeys a class with a private default constructor, instead of using an Interface ( just to follow the Johua's tips from Effective Java :P )

Rafael
[originally posted on jforum.net by Rafael Steil]

[Migrated From Jforum.net]

A complete pass over all configuration properties is always a nice moment to do some garbage collection. I found two dubious properties:
"useFilesystemCache" is not used anywhere
"avatarMaxKbSize" is only used in ./templates/admin/config_list.htm but never used anywhere so this limit is probably not enforced yet.
You can decide whether these are real problems (and therefore warrant an issue in JIRA) or not

[originally posted on jforum.net by Pieter]

[Migrated From Jforum.net]

Pieter wrote:An interesting approach, I don't know that book but I'm always interested to learn so can you give me the rationale behind this?

Here is the book site:
http://java.sun.com/docs/books/effective/

There are some sample chapters for download. This is a very good book. The argument for not using interfaces for constants only is much in the sense of "good practices". I mean, the argument is like "... when you use an interface just for storing constant values, it may not be clear the intention of that interface, and some developers may even try to implement the interface, which is not the goal. So, the better is to make it a public class with a private, non-argument, constructor, to prevent subclassing...".

The author also says why is better to use the private default construtor instead of makint the class "final", but I don't remeber exactly the point ;), so I don't have the book on my hands right now.

Pieter wrote:
If you choose to use an abstract class instead of an interface you cannot implement the interface to use the constants unqualified.

So, neither abstract. Just a regular class.

Rafael
[originally posted on jforum.net by Rafael Steil]

[Migrated From Jforum.net]

Pieter wrote:A complete pass over all configuration properties is always a nice moment to do some garbage collection. I found two dubious properties:
"useFilesystemCache" is not used anywhere
"avatarMaxKbSize" is only used in ./templates/admin/config_list.htm but never used anywhere so this limit is probably not enforced yet.
You can decide whether these are real problems (and therefore warrant an issue in JIRA) or not

Well, "useFileSystemCache" was intended to tell the system if to use the cached .htm files of topics, messages and so on, instead of processing the pages on every request. Of course this feature doesn't exists yet :P

The "avatarMaxKbSize" am not sure if it will be necessary some day, because the admin could specify the max width and height of images, which makes the images tiny when necessary.

And yes, you could drop both keys for now.

Rafael
[originally posted on jforum.net by Rafael Steil]

[Migrated From Jforum.net]

I've implemented the changes I proposed. Because this required a lot of little changes
all over the code, it might be a good idea to get these changes into the repository as
quickly as possible to minimize the possible conflicts with any work you are doing.

Do you want me to commit these changes directly in the repository (I will need
write access to do so of course), or do want me to send you the patches?

To make ik truly possible to separate developers, we still need some solution for
database specific properties. I propose to merge these properties in the generic
set of configuration properties.

Instead of having mysql specific properties in a separate file, we could just put them
in the main configuration but with a different name, so:
database.connection.driver = com.mysql.jdbc.Driver
becomes:
database.mysql.connection.driver = com.mysql.jdbc.Driver

This way we can still maintain several database configurations but we are not
forced to use different files. Given my patches, individual developers can then
easily have their own database settings, even for several different databases
if they need.

If you agree, I have no problem implementing this change.

[originally posted on jforum.net by Pieter]

[Migrated From Jforum.net]

Ok, I will give you Developer access to the CVS. I need your username at java.net to do that.

On the other question, yesterday I changed the config files so the user ( "user" here means the admin / owner ) just need to inform the database name in database.properties, and the system will search for the configurations at "database/<driver.name>/*".

As database.properties just have one key, it may be moved to SystemGlobals, which will let us use the "per developer configurations", correct?!..

Ahhh, but we still have the problem with usernames, hosts and passwords... hhmm, and how about doing the same thing to <driver.name>.properties as you do in SystemGlobals? ( per dev config ).
I guess this approach is better than having different keys in SystemGlobals for each different database..

Rafael
[originally posted on jforum.net by Rafael Steil]

[Migrated From Jforum.net]

My account name on java.net is Pieter2 (pieter was taken :?).

You are right that most stuff should stay in database/<driver>/

But introducing yet another user dependent file does not make it easier to configure
JForum for a new user.

If we place "driver.name" and "database.connection.string" in the main configuration
our troubles are over: all the other properties are unlikely to be different between
installations.

On second thought it might be an idea to also move the other 4 properties in
mysql.properties to the main configuration because 3 of them are not mysql
specific as far as I can tell. The fourth (database.connection.driver) is, but keeping
a complete file for this one property seems like overkill.

The other three files (mysql.sql, mysql_data_dump.sql, and mysql_db_struct.sql)
should clearly be left in tact

This way, when a new user wants to install JForum, the only editing he/she needs
to do is to create a file conf/<user>.properties and put in about three properties:
- forum.link (as it is unlikely that JForum should run on the localhost),
- database.driver.name (only if not using mysql)
- database.mysql.connection.string
As this is all configuration stuff that is needed to get JForum up and running,
and all other stuff can be edited online by the admin!

Note that with a fresh install it is also possible to just edit these properties in the main configuration file, and not force the installer to create a new file. The user specific file is especially useful to separate developers.

I have actually added experimental ${key} replacement in the configuration options,
so we could even simplify things further by adding:
database.mysql.connection.string=jdbc:mysql://${database.mysql.connection.host}:${database.mysql.connection.port}/jforum?user=${database.mysql.connection.username}&password=${database.mysql.connection.password}&autoReconnect=true
database.mysql.connection.host=localhost
database.mysql.connection.port=3306
database.mysql.connection.username=<username>
database.mysql.connection.password=<password>

Now instead of heaving to grasp the complete connection string, most users only need to provide the username and password options. Power users that need complete control over the connection string can still provide one in their properties!

[originally posted on jforum.net by Pieter]

[Migrated From Jforum.net]

Pieter wrote:
But introducing yet another user dependent file does not make it easier to configure
JForum for a new user.

If we place "driver.name" and "database.connection.string" in the main configuration
our troubles are over: all the other properties are unlikely to be different between
installations.

Ok, I aggree.

Pieter wrote:
On second thought it might be an idea to also move the other 4 properties in
mysql.properties to the main configuration because 3 of them are not mysql
specific as far as I can tell.

[...]

This way, when a new user wants to install JForum, the only editing he/she needs
to do is to create a file conf/<user>.properties and put in about three properties:
- forum.link (as it is unlikely that JForum should run on the localhost),
- database.driver.name (only if not using mysql)
- database.mysql.connection.string
As this is all configuration stuff that is needed to get JForum up and running,
and all other stuff can be edited online by the admin!

Seems reasonable. But am not sure of putting all database related stuff in SystemGlobals. Until sometime ago, it was all in config/database.properties, which only kept configuratios for a single database, resulting in the move to database/<driver>/* .

As is simpler to manipulate the merges when all stuff in in SystemGlobals, it is not very clear to the user to have the database configs there. Moreover, if we add support for other databases, the file will be bigger than necessary, containing much non-related keys.

So, I guess it is better to move back to database.properties, but then using your suggestion of

database.<driver>.connection.host = xxx
database.<driver>connection.username = yyy

and so on. This keep the system in a organized fashion ( or kinda ), and still lets the multiple database configuration.

Pieter wrote:
I have actually added experimental ${key} replacement in the configuration options,
so we could even simplify things further by adding:
database.mysql.connection.string=jdbc:mysql://${database.mysql.connection.host}:${database.mysql.connection.port}/jforum?user=${database.mysql.connection.username}&password=${database.mysql.connection.password}&autoReconnect=true
database.mysql.connection.host=localhost
database.mysql.connection.port=3306
database.mysql.connection.username=<username>
database.mysql.connection.password=<password>

Ok.. but just add a key for the database name too.

For the per user configurations, we can still have one single file for that ( config/<user>.properties, using your example ). This file will store all specific configuration, be it database of general configuration data. We just need to make a little change to ConnectionPool in order to parse this file too, or something like that ( anyway, much probably I will change the connection pool and make the general process a litte better, since it is getting confusing ).

Ah, I need you to request "Developer" access to JForum. Please go to http://jforum.dev.java.net and use the link "Request project membership/role".

Rafael
[originally posted on jforum.net by Rafael Steil]

[Migrated From Jforum.net]

I've just ploughed through about 80(!) conflicts when updating your changes

I ended up with a merged version that the compiler accepts, but I still need to test it.

As soon as I have some confidence that this version is working more or less correctly
I will commit my changes.

Let's first make sure we have a version that is working correctly and then implement
the database changes as you propose.

I don't think we need to reparse the user specific file in the ConnectionPool class, we can just get the property values using SystemGlobals.getValue.

[originally posted on jforum.net by Pieter]

[Migrated From Jforum.net]

As far as I can tell, everything seems to work so I took the plunge and committed all my changes.

Early on I decided to rename a handful of properties. I thought it might make the naming a little bit more systematic, but looking back this was clearly a mistake. If you dislike any of the new property names, please let me know and I will rename them back. Renaming them was more work than I initally thougt, but It did learn me a thing or two about the code .

During the merge operation, I didn't encounter the 'database.name' property you where talking about, so I hope I didn't loose any of your modifications ops:
[originally posted on jforum.net by Pieter]

[Migrated From Jforum.net]

I updated your changes from CVS.. am taking a closer look, but seems nice.
There is only two points I ask you:

1) Please use tabs instead spaces for indenting ( 4 length is ok ). I don't care if you use Sun code style or Jakarta style, but, *please*, use tabs

2) Put the copyright on every new file ( you can copy it from any other java source ). Ah, and add

@verion $Id: $

as well, so we can know exactly which class' version are we using.
[originally posted on jforum.net by Rafael Steil]

[Migrated From Jforum.net]

Very nice code Pieter, great job :-D

From my part, I commited some changes to the CVS, including the copyright thing and the identation. I also fixed some other bugs that were assigned to me on JIRA, and opened another couple :P

There are very few itens to finish before releasing RC4!!

Rafael
[originally posted on jforum.net by Rafael Steil]

[Migrated From Jforum.net]

Rafael Steil wrote:I updated your changes from CVS.. am taking a closer look, but seems nice.
There is only two points I ask you:

1) Please use tabs instead spaces for indenting ( 4 length is ok ). I don't care if you use Sun code style or Jakarta style, but, *please*, use tabs

2) Put the copyright on every new file ( you can copy it from any other java source ). Ah, and add

@verion $Id: $

as well, so we can know exactly which class' version are we using.

No problem. I have checked the "Use tab character" checkbox in the eclipse
"Preferences -> Java -> Code Style -> Code Formatter -> Edit -> Indentation" page.
I do hope the eclipse team someday adds the possibility to have different formatting styles for different projects. On second thought, I probably can achieve almost the same effect by opening the jforum project in its own workspace.

It might be a good idea to export your formatting settings and commit the resulting xml file into the jforum CVS repository. Developers can then just import these settings and be sure they have the same formatting styles you prefer!

You can do the same for the Code Templates: if you add the copyright and @version $Id: $ stuff, we will get them automatically for each new file!

[originally posted on jforum.net by Pieter]

[Migrated From Jforum.net]

I have marked my issues as 'Resolved' in JIRA, if you rather want me to close them directly in the future just let me know, but it seems fitting that you decide whether an issue is really closed.

[originally posted on jforum.net by Pieter]

[Migrated From Jforum.net]

Closing JF-13 (per user config) maybe was a little premature as the database stuff is still not done. Shall I reopen this issue and work on this some more?
[originally posted on jforum.net by Pieter]

[Migrated From Jforum.net]

Pieter wrote:It might be a good idea to export your formatting settings and commit the resulting xml file into the jforum CVS repository. Developers can then just import these settings and be sure they have the same formatting styles you prefer!

Code formating is a very personal thing, and this subject can result in really hot flame war between developers, when trying to "decide" which style to use.
So, I often really don't bother the team to follow a very rigid rule, as long they write good and clean code, and comment it when necessary. The tab char is the only question I ask to follow!

Pieter wrote:
You can do the same for the Code Templates: if you add the copyright and @version $Id: $ stuff, we will get them automatically for each new file!

It is an interesting idea, but this ends on a Eclipse's leak: it is not possible ( or I don't know how to do ) to have, as you said, different styles for different projects. So, putting the license in code templates will result on having that many lines of license code on every new file, even when not wanted.
Nowadays I just copy & paste the license.. it just takes a few seconds to do that..

Rafael
[originally posted on jforum.net by Rafael Steil]

[Migrated From Jforum.net]

Pieter wrote:Closing JF-13 (per user config) maybe was a little premature as the database stuff is still not done. Shall I reopen this issue and work on this some more?

Is up to you ;)

Rafael
[originally posted on jforum.net by Rafael Steil]