Current development on JAMWiki is primarily focused on maintenance rather than new features due to a lack of developer availability. If you are interested in working on JAMWiki please join the jamwiki-devel mailing list.

Tech:User Preferences

ktip.png This page (and all pages in the Tech: namespace) is a developer discussion about a feature that is either proposed for inclusion in JAMWiki or one that has already been implemented. This page is NOT documentation of JAMWiki functionality - for a list of documentation, see Category:JAMWiki.
Status of this feature: Implemented for Tech:JAMWiki 1.3.
Contents

Description[edit]

User preferences are current stored in the table jam_wiki_user as a single column per preference. As for version 1.2.x there are only 4 user preferences, but there are plans to provide users with additional customization possibilities. This is the current schema of jam_wiki_user:

                                                Table "public.jam_wiki_user"
        Column         |            Type             |                              Modifiers
-----------------------+-----------------------------+----------------------------------------------------------------------
 wiki_user_id          | integer                     | not null default nextval('jam_wiki_user_wiki_user_id_seq'::regclass)
 login                 | character varying(100)      | not null
 display_name          | character varying(100)      |
 create_date           | timestamp without time zone | not null default now()
 last_login_date       | timestamp without time zone | not null default now()
 create_ip_address     | character varying(39)       | not null
 last_login_ip_address | character varying(39)       | not null
 default_locale        | character varying(8)        |
 email                 | character varying(100)      |
 editor                | character varying(50)       |
 signature             | character varying(255)      |

The user preferences are display_name, default_locale, editor and signature.

The target of this discussion is to establish which strategy should be followed for adding user preferences.

Proposed changes[edit]

There are at least 3 approaches that are indicated as best practices:

  1. One preference table (could be jam_wiki_user) with a preference per column. This is the current approach and some authors refer to it as the best solution, because it allows selective search on user preferences in a very straightforward way. The drawback, as mentioned already, is that adding new preference items requires a mofification in the database schema.
  2. One single preference column in jam_wiki_user containing a representation of the preferences (key=value/XML/...). This was the old MediaWiki approach. It keeps the database design simple, but is less flexible when searching for specific preference values.
  3. A preference table such as the current MediaWiki approach. Some authors state that performance could become an issue. It is also true that those complaining about performance seem to be reading preferences on the fly when they are needed. I guess that loading them at login and reload them at change would strongly reduce performance issues.

A possible layout for user properties would look like this - Notice that the it is not clear yet how MediaWiki deals with default values. In this proposal we assume a dedicated table for them:

 jam_user_preferences_defaults
 +----------------+------------------+
 | Field          | Type             |
 +----------------+------------------+
 | pref_key       | varchar(100)     |
 | pref_value     | varchar(250)     |
 | pref_group_key | varchar(100)     |
 | req_nr         | integer          |
 +----------------+------------------+

 Primary key (pref_key)

 jam_user_preferences
 +--------------+------------------+
 | Field        | Type             |
 +--------------+------------------+
 | wiki_user_id | integer          |
 | pref_key     | varchar(100)     |
 | pref_value   | varchar(250)     |
 +--------------+------------------+
 
 Primary key (wiki_user_id,pref_key)
 Foreign key (wiki_user_id) references jam_wiki_user (wiki_user_id)
 Foreign key (pref_key) references jam_user_preferences_defaults (pref_key)

Changes in the database[edit]

See UPGRADE.txt

Changes in the application software[edit]

The changes to move user preferences to a dedicated table are now implemented. The setup process for a new installation works fine on HSQL and PostgreSQL. The upgrade process is still in progress.

  • The functionality is in the container org.jamwiki.model.Wikiuser and in the database facing methods.
  • All occurrences of the "old" methods for storing and retrieving user preferences habe been updated:
    • get/set<PreferenceName> to getPreference(<name.of.preference>) and setPreference(<name.of.preference>, <value.of.preference>)
    • In JSTL: <c:out value="newuser.preferences['name.of.preference'].
  • New users receive the default user preferences as defined in jam_user_preferences_defaults table. Actually the default values are not stored on a per user base. That means that the size of the table only increases if users actually set any preference other than the default ones.
  • Default user preferences that are set to null receive the value of an application property, if any available, e.g. user.datetime.format has the value of the property signature_date.

Additions

  • Two new preferences:
    • User timezone.
    • User datetime format (encoded as java.text.SimpleDateFormat pattern String).
  • The WikiSignatureTag class has been modified to use the user timezone.

The code is not committed yet, because I still have a problem with the automatic upgrade process.

Update: The problem is solved. I will make some additional improvements and testing and finally commit the changes to branches/cclavadetscher for review.

Changes in other files[edit]

  • The file UPGRADE.txt has been updated to include instruction for manual upgrade of the database.

Tests[edit]

  • Install from scratch (setup): successful with both HSQL and PostgreSQL.
  • Upgrade from JAMWiki 1.2.0 to 1.3.0 (upgrade): successful with both HSQL and PostgreSQL.
  • Functionality:
    • Default setup of user properties after installation works fine.
    • Default setup of user properties after registration works fine.
    • Custom definition and usage of the user preferences works fine.
    • Note: the insert method for WikiUser cannot simply set all properties to default, because user can set a locale during the registration process.

Issues[edit]

  • Unable to test with other DBMS than HSQL (internal) and PostgreSQL.

Author(s)[edit]

Status[edit]

  • 05.07.2012: Started discussion.
  • 10.07.2012: Ready for testing (local).
  • 11.07,2012: Commited to branch - ready for testing by other JAMWiki developers.
  • 15.07.2012: Merged to trunk.
  • 23.07.2012: Work-in-progress changes pushed to jamwiki.org
What does this mean, pushed to jamwiki.org? —The preceding comment was added by cclavadetscher (commentscontribs) .
During development I will occasionally push trunk code to jamwiki.org to help with debugging and solicit feedback. If you go to Special:Account you'll see your changes. -- Ryan • (comments) • 24-Jul-2012 08:18 PDT
  • 27.07.2012: Merged finalized version to trunk.
    • The latest code is now live on jamwiki.org. -- Ryan • (comments) • 15-Aug-2012 22:22 PDT

Commits[edit]

4101 Only to branches/cclavadetscher - Ready for review. -- Charles • 11.07.2012 15:33 CEST

4105 Merge to trunk Charles • 15.07.2012 15:38 CEST

4126 Merge to trunk: finalized re-engineering of user preferences.

4133 Solved object mismatch after upgrade due to caching -- Charles 31.07.2012 06:58 CEST.

4146 Flush connection pool when upgrading from previous versions -- Charles 31.07.2012 09:13 CEST.

Next steps[edit]

  1. Add new preferences based on users' requests.

Comments[edit]

Design notes[edit]

I think I like #3 - would that also be your choice? I'm not overly concerned about performance as it can be easily mitigated with the existing caching layer using the specified approach, and it looks like it would allow (for example) per-wiki customizations that wouldn't require schema changes. In addition, since the table can be indexed by user id if needed then updates and reads should be relatively fast. Let me know your thoughts, and thanks for getting this started. -- Ryan • (comments) • 05-Jul-2012 20:55 PDT

Yes. My first impression would also be to go for solution #3. It offers the highest flexibility. When you write "per-wiki customization, are you referring to virtual wikis on the same wiki instance? If so then there would be a need for an additional foreign key to the virtual wiki. However I am not sure that a user may really want to have different settings on different wikis...
I will start making first changes and tests to the software during next week. If you have additional input on the topic, you're welcome. -- Charles 15:03 CEST
Hello. About that problem with the automatic upgrade process. What is the purpose of forcing a login, before the upgrade starts? Thank you for feedback. Charles 10.07.2012 14:40 CEST
Problem solved. I was invoking the wrong method (executeUpgradeQuery instead of executeUpgradeUpdate). Charles 10.07.2012 15:33 CEST
Since the upgrade page is just a normal wiki page the login requirement is to ensure that only a site administrator performs an upgrade, and not just whoever happens to visit the page first. -- Ryan • (comments) • 10-Jul-2012 07:37 PDT
OK. That makes sense. -- Charles • 11.07.2012 15:14 CEST

Other question:

  1. I still don't see exactly what the preview ot the signature (and now potentially of the datetime format) should be. Do you think of an AJAX call to the server and a pop-up javascript window showing the formatting entered?
  2. Toggling the table of contents: is the idea that the user can choose to force a TOC on every page, i.e. before the threshold of three entries is reached? In that case a user may also want never to see a TOC on a page. At the end he would have the choice between:
  • Standard behaviour (null, not set)
  • Always add TOC to page (-1)
  • Never add TOC to page (0)
  • Add a TOC after a user defined number of title entries

Please elaborate on these :-)

Charles • 11.07.2012 16:02 CEST

  1. I can take care of the signature preview - it doesn't need to be Ajax, just display the current signature when the preferences page is loaded.
  2. The TOC toggle isn't a must-do, but a number of people have requested the ability to hide or collapse the TOC by default in the past (by default it displays fully expanded). Your suggestions for values make sense, and additions could be made in the future.
-- Ryan • (comments) • 11-Jul-2012 21:30 PDT
  1. OK. That's great. I got meanwhile the idea when I saw it in on WikiTravel. I will take care first of the other issues and come back to that if you did not have a chance to do it. Let's keep in touch on that.
-- Charles • 12.07.2012 09:03 CEST (BTW: This will happen automatically with the 1.3.0 release :)

I am thinking of a method to simplify the handling (actually the additions) of new preferences. I am not sure it works using JSTL, but we have done something similar in the company with very good results.

The idea is that the user preferences management page basically always has the same elements:

  • A label for the preference.
  • An input field or a dropdown box (could also be a checkbox in the future for boolean values) depending on the preference peculiarities.
  • An optional preview.
  • An optional help text.

Let's suppose that the naming for passing elements to JSTL is standardized:

  • Base name: the name of the preference in jam_user_preferences_defaults.
  • The label for the input field: <Base name>.label
  • The input field: null = input type text; <Base name>.selection = a drop down box (with key/value pairs); <Base name>.list = a drop donw box (without keys); <Base name>.checkbox = a checkbox.
  • The optional preview: <Base name>.preview.
  • The optional help text: <Base nane>.help.

Example 1:

  • Preference default locale (the select has key/value pairs)
    • base name: user.default.locale
    • label: user.default.locale.label
    • input: user.default.locale.selection
    • preview: null
    • help: user.default.locale.help

Example 2:

  • Preference timezone (the select is only a list of IDs)
    • base name: user.timezone
    • label: user.timezone.label
    • input: user.timezone.list
    • preview: null
    • help: user.timezone.help

-- Etc...

Now register.jsp could loop over the preference keys and depending on the content type display the elements as now. The difference would be that adding preferences would not require any changes in the GUI.

I could make a test implementation of that concept and see if it fits with JSTL.

One question that arises is, if the jam_user_preferences_defaults table should have an additional attribute seq_nr (sequence number) to control the position in which the preference should be displayed.

Another question is, if it would make sense to have a UserPreference interface to simplify even more the handling of preferences.

What do you think of this idea? Did I explain the concept clearly?

Charles 16:07:2012 08:20 CEST

I made some testing on this topic. It is not possible to base the whole concept only on labelling. The only way would be to create a UserPreferences class that implements getters for the necessary objects (lists, hashmaps and the like).

Charles 16:07:2012 18:33 CEST

Code review comments[edit]

A few comments on the code:

  • Looks good for the most part.
  • I think it would be best to leave display_name as a field in the jam_wiki_user table since it isn't really a preference - it probably wouldn't ever have a non-null default. Does that make sense to you? Sorry for not bringing this up earlier.
Yes, I agree and don't worry, this is a minor change compared with the rest. In that case I would also move the input field from the User preferences to the Account Detais section in the Special:Account page. OK for you?
  • The transaction code in AnsiQueryHandler is unnecessary I think - it should be covered by AnsiDataHandler.writeWikiUser().
Well, I guess that you are right. I will take a closer look at that.
  • In general it's best practice to have methods return an interface rather than a class, so in the new code return Map instead of HashMap (see [1] for reasoning).
Correct. I will change that.
  • It would be better to define constants for the standard preferences ("user.default.locale") to avoid hard-coding.
Dito.
  • For simplicity it might make sense to keep some of the methods like WikiUser.getSignature() and use them as a shortcut for WikiUser.getPreference("user.signature"). This would reduce the number of changes required in other code, but would only be useful for common preferences such as signature and locale - more generic preferences (such as editor or TOC) could be retrieved via map key as you're doing now.
Yes. That was my first implementation, before changing all calls to setters and getters. For some preferences it may really be a better way to go, since it makes programming easier (don't need to remember constant names).
  • I think the changes to JAMWikiAuthenticationProcessingFilter and JAMWikiPostAuthenticationFilter were committed accidentally - they don't appear to be related to the preferences changes. Never mind, this appears to be a merge from trunk that was included in the commit.

Overall this looks great and should be a huge improvement - thanks, and please let me know if I can help out! -- Ryan • (comments) • 11-Jul-2012 21:30 PDT

Thank you for reviewing! See my comments inline above. Bye -- Charles • 12.07.2012 09:10 CEST
All changes are made, so far. I had the following strange behaviour however:
  1. The upgrade only seems to work if I empty first the file-system directory. This is puzzling because when I made my tests yesterday I didn't need to do that. Is it a normal procedure to delete something in the file-system directory? I guess that internal database uses the same directory for storage, which makes it a bit useless to have to empty it first... Maybe if you find some time you may have a look at that.
  2. On one occasion I got the exception that all connections to the database were busy. I am not sure if this depends on the fact that during developement I continously restart tomcat. This may cause open connections to be closed unclean. Anyway that just happened once. I think that some load testing may bring an answer, if this is an issue or not.
  3. I was not able so far to find a simple way to use java constants (as declared now in WikiUser) in JSTL. Do you have an idea? Did you use this feature somewhere else?
OK. I hope that helps anyway for the moment. Bye -- Charles 12.07.2012 17:07 CEST
I've got company in town tonight and tomorrow, so I'll need to look into the issues above over the weekend, but regarding #3, I don't think there is an easy way to use constants from JSTL, so hard-coding there should be fine - I hard code retrieval of environment properties from the admin pages. I'll follow-up regarding the other points over the weekend. Thanks for your quick turnaround on the code review! -- Ryan • (comments) • 12-Jul-2012 19:37 PDT
Let me be more specific on point 1. The upgrade worked in the sense that the database was modified correctly and I could log in as admin. A click on "account" or on "register" before logging in, i.e. a call to Special:Account brought the error message that the property defaultLocale was not available. This property was used in 1.2.x. So it seems that the register.jsp was not updated. I repeated the migration process test this morning and it worked fine. I think it may have been a cache problem, althought restarting tomcat should actually clear it. I may be able to do some more testing later in the afternoon. If so, I will post the results here.
Another note on point 2. A change that may influence the behaviour: In the insert and update methods I reuse the statement object for all calls to the DB. This should not be an issue, but the docs suggests that it is a good policy to close statements after each use. So I will try to modify the code in that way, just to make sure.
Enjoy the company :-) -- Charles 13.07.2012 07:27 CEST
Update: I made the changes mentioned above and added a preview for the datetime format. Besides usual testing, what is still missing is the preview for the user signature. I will leave that to you. At a first glance I was not able to find what method to call to format the signature string for displaying. I think that you will get much quicker to that. Bye -- Charles 13.07.2012 10:18 CEST
The latest changes look great - feel free to merge this to trunk whenever you're ready, and I'll then add signature preview. One very minor issue in AnsiQueryHandler.insertWikiUser and AnsiQueryHandler.updateWikiUser:
	if(!StringUtils.isBlank(defVal)) {
		if(!defaults.get(key).equals(preferences.get(key))) {
			stmt.setInt(1, userId);
			stmt.setString(2, key);
			stmt.setString(3, cusVal);
			stmt.executeUpdate();
		}
	}
	else {
		stmt.setInt(1, userId);
		stmt.setString(2, key);
		stmt.setString(3, cusVal);
		stmt.executeUpdate();
	}
...could be simplified to:
	if (StringUtils.isBlank(defVal) || !defaults.get(key).equals(preferences.get(key))) {
		stmt.setInt(1, userId);
		stmt.setString(2, key);
		stmt.setString(3, cusVal);
		stmt.executeUpdate();
	}
Also, in AnsiQueryHandler.updateWikiUser it looks like you accidentally removed the DatabaseConnection.closeStatement(stmt) call, which may be the source of the "out of connection" errors you mentioned.
As to #1 above, Tomcat often fails to properly update cached JSPs. If you empty the $TOMCAT_HOME/work/Catalina directory does the problem go away?
Thanks again, and let me know if there are any problems while merging. -- Ryan • (comments) • 14-Jul-2012 15:52 PDT
I made the change that you proposed, merged the changes in my local version of trunk and repeated all tests with the trunk version. Everything seems to work fine. I had some problems during merging, but not with trunk. After merging I wanted to synchronize the working dir of my branch with trunk. I received the error that a local add with an add from merge conflicts (DateUtil.java). According to the svn docs in those cases you need to svn delete the file in the working branch and repeat the merge. After doing that the add from merge was not done. So I ended up with a missing class, which I finally svn added manually. I guess that I will need to get a bit deeper into tree conflict resolution with svn. This is to explain the additional commits made to my branch. BTW: I did not modify the CHANGELOG.txt so far. Should I do it or is it more a PL task :-) ?
Additional notes: The missing DatabaseConnection.closeStatement(stmt) is quite sure the source of the problem I had. Also emptying $TOMCAT_HOME/work/Catalina (finally /var/cache/tomcat6/Catalina) helped solving the cache problem. I could reproduce the error during testing.
Bye, Charles 15.07.2012 11:11 CEST
Question: A while ago in a note you wrote that we should not commit things to our branches that are not intended for merge to trunk. As a matter of fact synchronizing local working branches with trunk leads sometimes to a list of updates that need to be committed. Is there a better way to keep synchronized with trunk? Thank you for advice. Charles 15.07.2012 11:19 CEST
  1. I'm not a Subversion expert, but I've found that doing an "svn update" prior to a merge sometimes helps avoid conflicts, even if the branch hasn't changed.
  2. You're welcome to update CHANGELOG and CREDITS as needed.
  3. The guideline on not committing changes to a branch that aren't intended for merge is to avoid having branches where commits must be cherry-picked for merging to trunk. So, for example, if you wanted to try out a new UI that wasn't necessarily going to be merged to trunk, create a separate personal branch from the one that you use for changes meant for trunk - I've done this with branches/wrh2/fckeditor (not for immediate merging) and branches/wrh2/wip (stuff that will be merged).
  4. The database upgrade code worked great - that can be tricky to get right, so nice job!
  5. I made three minor changes to your code: the SQLException(Throwable) constructor doesn't compile with Java 5 (it was added in Java 6) so I removed that, I added a license header to the new DateUtil class, and during upgrade it gave me an incorrect table name for the data updates due to extra arguments being passed to MessageFormat.
  6. One bug that I found was that if you enter an invalid date pattern as a user preference then things break due to an IllegalArgumentException from DateUtil. Adding a check to RegisterServlet.validate() should fix that, but when I started to do so I found a couple of minor refactoring improvements that could be done to DateUtil. I may try to put these in place tonight (time permitting), but if you have any concerns please let me know.
  7. I'll also try to get the signature preview in place tonight.
Once again, thanks for the contribution, and please let me know if there is anything else that I can do. -- Ryan • (comments) • 15-Jul-2012 18:17 PDT
4: No concerns at all. Thank you for pointing these "problems" out.
5: Ups... quite bad. Let me know if you plan to fix that. Otherwise I could take over. Which improvements are you thinking of for DateUtil?
Update: Actually there are two ways to avoid the exception. Either checking the format, which is the case if users can use any format or provide a list of available formats (these could be shown in the dropdown box already as they would display). Is it a good idea to allow users to define the format completely free? Techies will have no problem, but what about non technical affine users?
6: I have seen that this in now implemented. However it seems that there is a preview only if the user entered something. Wouldn't it be good to display the default signature? I think this is [[User:user|displayName or login]].
I also made a change to fix a bug: The modification in insert-/updateWikiUser that you suggested above works fine, but I forgot to remove the "else" portion. The net effect is that preferences are always written to the database, which is what I wanted to avoid.
Charles 16.07.2012 07:25 CEST
revision 4113 adds validation. The only changes I made to DateUtils was to split things up a bit to help make it easier to follow the code. To your point about allowing users to select format from a list, that's how Mediawiki does it, and that may be a better option since it will prevent errors and also avoid the need to handle things like HTML in the date pattern - I'd be in favor of adding that change. Regarding a default signature, that will require some work with the parser, since JAMWiki supports multiple parser implementations and each handles signatures differently - let me give it some thought. -- Ryan • (comments) • 16-Jul-2012 23:00 PDT
revision 4117 updates the preview UI to render more cleanly, especially for long preview values. -- Ryan • (comments) • 23-Jul-2012 21:11 PDT
Hello. I have been a bit busy last days and it will keep on some more days. Still I hope that I can get to some more contribution. I can take over the generation of the list of allowed datetime formats, if you didn't start the task yet. I will have a look at how MediaWiki does. What I have seen on WikiTravel is a list with radio buttons. Honestly I don't find it a good solution, because it takes up a lot of screen space. I think that a dropdown box with dates formatted based on set of defined formats would be better. Your opinion? Next question is which formats should be used. I'd like to spend some time investigating if there is a way to use the locale to retrieve a set of country specific standard datetime formats. But again, this may take a few days. Not a big change, but little time.
Another question: I was thinking about a UserPreferences class to simplify handling of preferences so that developer can focus on the functionality instead of the GUI. In that process I noticed (well I noticed that before, but it had no effect) that model classes don't interact with the database. Is this a design decision? I was thinking that it could be interesting when I call a model constructor that the class manages its data itself, e.g. filling itself up with all data available from the database or not, if there is not an object for it. The thought came to me because of the UserPreference class, which I would implement as a model class, but would like to have it behave quite intuitive. So if I create a UserPreference object it should already have all information that is needed from the database. Kind of an ORM idea, but in some cases this could bring advantages. If this is by design impossible, I would opt for a utility class instead.
Charles 18.07.2012 18:30 CEST
First, definitely don't apologize for having to take a few days to do other things - I've been sidelined with other projects for most of the past two months :) . Second, I haven't done any further work on implementing date formats from a list, so if you'd like to work on that please go ahead.
As to pre-populating model objects, there are performance concerns to consider, which is one of the reasons why the current code separates the "data handler" from the "model" object (another reason is that this code is quite old, and pre-dates the switch to using the Spring framework). If pre-populated model objects were going to be implemented then I'd suggest looking into using Spring's model objects, which do basically what you're asking about. However, that would be a pretty huge change and something that would require a lot of investigation to ensure it could be done in a way that wouldn't break existing installations during upgrades, wouldn't introduce performance or other issues, etc. I'm definitely not opposed to the idea, but given the amount of work required to implement, test, and debug there would need to be enough advantages to warrant the effort. -- Ryan • (comments) • 18-Jul-2012 10:15 PDT
I added the DEFAULT dateformat type to the DateUtil class. I had a look at the datetime format available by default, i.e. SHORT, MEDIUM, LONG, FULL and DEFAULT and as long as I can judge they support locale specific formatting. However there is at least one point that I don't like. Some formats don't append the timezone, which is, from my point of view, absolutely mandatory as soon as contributions are written by people living in completely different parts of the world. So I suggest to display these "standard" choices plus 2 more proprietary and to add the timezone where it's missing. Here some example how that would look like with different locales:
en: 
SHORT                 :  7/19/12 7:28 PM CEST
MEDIUM                :  Jul 19, 2012 7:28:04 PM CEST
LONG                  :  July 19, 2012 7:28:04 PM CEST
FULL                  :  Thursday, July 19, 2012 7:28:04 PM CEST
DEFAULT               :  Jul 19, 2012 7:28:04 PM CEST
dd-MMM-yyyy HH:mm zzz :  19-Jul-2012 19:28 CEST
dd.MM.yyyy HH:mm z    :  19.07.2012 19:28 CEST
 
en_US: 
SHORT                 :  7/19/12 7:28 PM CEST
MEDIUM                :  Jul 19, 2012 7:28:04 PM CEST
LONG                  :  July 19, 2012 7:28:04 PM CEST
FULL                  :  Thursday, July 19, 2012 7:28:04 PM CEST
DEFAULT               :  Jul 19, 2012 7:28:04 PM CEST
dd-MMM-yyyy HH:mm zzz :  19-Jul-2012 19:28 CEST
dd.MM.yyyy HH:mm z    :  19.07.2012 19:28 CEST
 
es_US: 
SHORT                 :  7/19/12 7:28 p.m. CEST
MEDIUM                :  jul 19, 2012 7:28:04 p.m. CEST
LONG                  :  19 de julio de 2012 7:28:04 p.m. CEST
FULL                  :  jueves 19 de julio de 2012 7:28:04 p.m. CEST
DEFAULT               :  jul 19, 2012 7:28:04 p.m. CEST
dd-MMM-yyyy HH:mm zzz :  19-jul-2012 19:28 CEST
dd.MM.yyyy HH:mm z    :  19.07.2012 19:28 CEST
 
es: 
SHORT                 :  19/07/12 19:28 CEST
MEDIUM                :  19-jul-2012 19:28:04 CEST
LONG                  :  19 de julio de 2012 19:28:04 CEST
FULL                  :  jueves 19 de julio de 2012 19H28' CEST
DEFAULT               :  19-jul-2012 19:28:04 CEST
dd-MMM-yyyy HH:mm zzz :  19-jul-2012 19:28 CEST
dd.MM.yyyy HH:mm z    :  19.07.2012 19:28 CEST
 
de: 
SHORT                 :  19.07.12 19:28 MESZ
MEDIUM                :  19.07.2012 19:28:04 MESZ
LONG                  :  19. Juli 2012 19:28:04 MESZ
FULL                  :  Donnerstag, 19. Juli 2012 19:28 Uhr MESZ
DEFAULT               :  19.07.2012 19:28:04 MESZ
dd-MMM-yyyy HH:mm zzz :  19-Jul-2012 19:28 MESZ
dd.MM.yyyy HH:mm z    :  19.07.2012 19:28 MESZ
 
de_CH: 
SHORT                 :  19.07.12 19:28 MESZ
MEDIUM                :  19.07.2012 19:28:04 MESZ
LONG                  :  19. Juli 2012 19:28:04 MESZ
FULL                  :  Donnerstag, 19. Juli 2012 19:28 Uhr MESZ
DEFAULT               :  19.07.2012 19:28:04 MESZ
dd-MMM-yyyy HH:mm zzz :  19-Jul-2012 19:28 MESZ
dd.MM.yyyy HH:mm z    :  19.07.2012 19:28 MESZ
 
it: 
SHORT                 :  19/07/12 19.28 CEST
MEDIUM                :  19-lug-2012 19.28.04 CEST
LONG                  :  19 luglio 2012 19.28.04 CEST
FULL                  :  giovedì 19 luglio 2012 19.28.04 CEST
DEFAULT               :  19-lug-2012 19.28.04 CEST
dd-MMM-yyyy HH:mm zzz :  19-lug-2012 19:28 CEST
dd.MM.yyyy HH:mm z    :  19.07.2012 19:28 CEST
Next point is where to store the information on the choices. One possibility is in the database. This makes things flexible, but requires a new table and at least a selecting method. The other way is to hardcode it into the DateUtil class. I would be more in favour of the first solution (but then for me all information should be in the database). Considering that these values are not going to change quickly I could even accept the second solution. From an effort point of view it is not a big difference.
Let me know what you think. Thx&Bye -- Charles 19.07.2012 19:34 CEST
I have now a version with the permissible formats in a string array in DateUtil:
private static final String[] dateFormats = new String[]{ "SHORT","MEDIUM","LONG","FULL","dd.MM.yyyy HH:mm z" };
The first item in the list is prepended and set to the JAMWiki property default. You may also notice that DEFAULT is not in the list. It seems to be always identical to LONG and so it makes no sense to have it. I also modified the help text and the preview is not necessary anymore.
I will make some more testing tomorrow and then commit the changes.
Charles 20.07.2012 03:35 CEST
I think the string array makes more sense than a new database table - I try to keep the number of tables to a minimum since there is less control over them; Java code either compiles or doesn't, but different databases may have slightly different syntax, so it's often easier to just avoid dealing with a table when not required. -- Ryan • (comments) • 19-Jul-2012 20:15 PDT

UserPreferencesUtil[edit]

The change is done. I also made a major improvement. I would appreciate if you could have a look at it. Here is a description:

 User Preferences Management.
 
 * New class UserPreferencesUtil: this class is a container for user preferences. It simplifies
   the handling of user preferences in the GUI. 
 
 New process for adding user preferences: Step by step with a fictional example.
 
 Let's suppose that we want to add a user preference recording the preferred food.
 
 1. Define a default entry in jam_user_preferences_defaults: Note that preferences belong to groups.
    You can see a list of the available groups with

    select distinct pref_group_key
    from jam_user_preferences_defaults;
 
    You may also define a new group using a key of the type user.preferences.group.<new group>.
    Additionally you must define a position within the group (e.g. 1).

    insert into jam_user_preferences_defaults (pref_key, pref_value, pref_group_key, seq_nr)
    values ('user.preferred.food',null,'user.preferences.group.gastronomy',1);
 
 2. Define a constant in WikiUser:
 
    public static final String USER_PREFERENCE_PREFERRED_FOOD = "user.preferred.food";

    Note: If you defined a new group then you also must add a constant in the class UserPreferencesUtil:

    public static final String USER_PREFERENCES_GROUP_GASTRONOMY = "user.preferences.group.gastronomy";
 
 3. Define the value range of the preference if necessary.
 
    If we wanted to let the user decide freely, there is no need to define anything. The default
    display is an input field of type text.
    
    In this example we want to limit the choices of the user to a few items. To do that we can
    use a list of Strings or a Map<String, String>. Depending on the choice you will need to
    add an if statement either to
    
    - UserPreferenceItem.getList()
    - UserPreferenceItem.getMap()
    
    Notice that UserPreferenceItem is an inner class of UserPreferencesUtil, which is necessary
    in order to access selectively preference values from JSTL.
    
    We decide to limit the access using a list of Strings. So we add the following lines:
    
    else if(prefName.equals(WikiUser.USER_PREFERENCE_PREFERRED_FOOD)) {
        return new String[]{"Pizza", "Spaghetti", "T-Bone Steak"};
    }
 
 4. Add text for the GUI: In ApplicationResources.properties add 
 
    user.preferred.food.label=Preferred food
    user.prererred.food.help=This value is used to diplay food icons on the recipe pages 

    Note: If you created a new group, then you also have to add the group key in the text file, e.g.:

    user.preferences.group.gastronomy=Food and beverages settings
 
 5. If the field has a preview also add the necessary code to
    
    UserPreferenceItem.getPreview():
    else if(prefName.equals(WikiUser.USER_PREFERENCE_PREFERRED_FOOD)) {
       return ...;
    }
    
 6. Finally, in order to enable new setups and upgrades, proceed as follows:
 
    In WikiDatabase.setupUserPreferencesDefaults and:
    In DatabaseUpgrades.upgradeXXX() // XXX depends on the version number
    add:
    
    handler.writeUserPreferenceDefault(WikiUser.USER_PREFERENCE_PREFERRED_FOOD, null,UserPreferencesUtil.USER_PREFERENCES_GROUP_GASTRONOMY,1); 
 
 7. After these steps the preference is managed in the GUI without additional implementation or modifications
    of the JSP.
    The developer can focus on the usage of the preference in the application.

I had to use a workaround to integrate the signature preview. It would be nice if the preview could be generated from UserPreferencesUtil instead of being passed to the class in a set method. Maybe in the future there is an improvement on that :-)

I committed the changes to branches/cclavadetscher.

Thanks and enjoy the week-end -- Charles 20.07.2012 15:05 CEST

I like the idea behind this change, my major concern is putting it in the jamwiki-core package since it is used for front-end display and has dependencies on message keys. It would be best to keep jamwiki-core focused on core architecture, and move any tools used for display purposes to jamwiki-web. -- Ryan • (comments) • 21-Jul-2012 12:04 PDT

Good point. Do you have any preference about the package to use? I could move the class to jamwiki-web keeping the package name, i.e. org.jamwiki.utils or create a new package org.jamwiki.web.utils or org.jamwiki.gui.utils. I would tend for one of the first two. There will be maybe in the future other utility classes in the web package, that are not GUI related. The change itself is then not a big issue. If the package name changes I will have to modify import statements whereever they are used, but it's less than half an hour work. I could probably do it today. Let me know your thinking about the package name -- Charles 23.07.2012 08:09 CEST

I also would like your opinion about the ordering of user preferences. Now preferences are ordered alphabetically (according to pref_key). If developers should be able to influence the sequences it would be a good idea to add a sequence column in jam_user_preferences_defaults. The ordering would then look like ORDER BY seq_nr, pref_key. Charles 23.07.2012 08:13 CEST

I made a short test with the refactoring utility of Eclipse. UserPreferencesUtil constants are used in some core classes. The code does not compile, because the class is not availabe at that time. Moving the class to jamwiki-web will be a bit more work than I expected, but absolutely feasible Charles 23.07.2012 08:23 CEST

Finally I made the changes already. I moved the UserPreferencesUtil class to the new package org.jamwiki.web.utils and the constants for user preference keys back to WikiUser. Everything compiles correctly, works good and is committed to branches/cclavadetscher. If you'd prefer another name for the package (I don't know exactly the policy of JAMWiki) then just tell me. As for the other point (sequencing) I think that it would make things better. If developers cannot order preferences, then logically related preferences may end up far apart or developers would be forced to consider alphabetical ordering while naming preferences. The latter could be quite confusing. As I said I am open for discussion on these two points. Cheers Charles 23.07.2012 08:48 CEST

Thanks! I should have created a org.jamwiki.web.utils package a while ago, so I think the name is good. Let me give some thought to the ordering issue - I'll be on a plane this evening so will have plenty of time to review code and give this some thought. -- Ryan • (comments) • 23-Jul-2012 07:44 PDT
Regarding sequencing, rather than a numeric sequence it might make more sense to have something like a "preference group" key and a "group sequence" value. If the list of preferences gets large, that would provide a way to keep similar preferences together, but would not require re-ordering preferences. Is that what you had in mind? Or did you have another proposal? -- Ryan • (comments) • 23-Jul-2012 21:09 PDT

No, not exactly. I was thinking of a simple numeric sequence, although I have to admit that a group key may be interesting. It makes things a little bit more complicated. I noticed that using a Map as a return value does not keep the sequence as delivered by the SQL statement. Using a SortedMap does not really help. What I did was to add a method to retrieve a String[] of the preference keys. I then use this array to loop through the preferences. That means that enforcing order or grouping requires a second call to the DB. I can modify the table and the select statement to deliver the correct order taking into account a group ID.

It could look like this:

jam_user_preferences_defaults
+---------------+------------------+
| Field         | Type             |
+---------------+------------------+
| pref_key      | varchar(100)     |
| pref_value    | varchar(250)     |
| pref_group_id | integer          |
| seq_nr        | integer          |
+---------------+------------------+

and

SELECT pref_key, pref_value
FROM jam_user_preferences_defaults
ORDER BY pref_group_id, seq_nr, pref_key

Considering that this is not a page where all millions of users will access concurrently, I think that it should not be a performance issue. It simply is not elegant... Do you have some alternative suggestions?

Initial values for pref_group_id could be 1000, 2000, 3000, etc, in order to be able to insert groups in between without renumbering the whole table.

-- Charles 24.07.2012 17:18 CEST

I made the changes and committed them to branches/cclavadetscher. So far I am not doing very much with the pref_group_id. Possibilities are to display the group in its own fieldset or if this becomes a need, to dedicated pages.

-- Charles 25.07.2012 16:30 CEST

Seems reasonable. One question - would a string for the group key work? I'm thinking that in the future it may be possible to create plugins that have user preferences associated with them, so rather than having the plugin "guess" a numeric group key and hope it's unique it could instead use the plugin class name or something that would be similarly unique. Obviously this isn't an issue right now, but long term it may become a concern.
Also, if you need an ordered map try LinkedHashMap, which will keep elements in the same order in which they were added to the map. -- Ryan • (comments) • 25-Jul-2012 07:32 PDT

Hello. Great I will have a look at the LinkedHashMap. This may simplify matters enormously. Of course it is possible to use a string for the group key. Although you say that this in not an issue now I think that to prepare the framework as good as possible now is the best anyway, since I am already working on that :-) So I will look into that -- Charles 25.07.2012 16:56 CEST

I made the changes and it looks good. Here a screenshot of the current status:

user-preferences.png

Tomorrow I will make the usual tests (setup and migration with HSQL and PostgreSQL) and commit the changes to branches/cclavadetscher. I guess that you would like to check my code before I merge to trunk. For the merging I may need some help anyway. If I run an svn merge --dry-run, I still get a tree conflict...

Here some more details:

  • I explained the whole idea already in previous posts. Basically my target is to enable developers that need to add some user configurable parameters to focus on the functionality instead of on the management of the preference itself.
    jam_user_preferences_defaults.pref_key       <-> public static final String WikiUser.PREF_KEY = ...
    jam_user_preferences_defaults.pref_group_key <-> public static final String UserPreferencesUtil.PREF_GROUP_KEY = ...
     
    The most important db access method is Map<String, Map<String, String>> getUserPreferencesDefaults()
     
    The return value is map representation of the jam_user_preferences_defaults table:
     
    Map(pref_group_key, Map(pref_key, pref_value))
     
    The UserPreferencesUtil class calls this method and transforms the return value into:
     
    Map<String, Map<String, UserPreferenceItem>> getGroups()
     
    This maps to:
     
    Map(pref_group_key, Map(pref_key, an instance of UserPreferenceItem for pref_key))
     
    UserPreferenceItem is an inner class of UserPreferencesUtil and declares public methods that can be used in JSTL:
     
    * public String getKey() : returns the pref_key that was used to initialize the instance
    * public String getLabel() : returns the pref_key value with the string ".label" appended. This label must be available in ApplicationResources_xx.properties and is used for display in the GUI.
    * public String getHelp() : returns the pref_key value with the string ".help" appended. This label must be available in ApplicationResources_xx.properties and is used for display in the GUI.
    * public String[] getList() : returns a String[] array if the preference must display a drop down box on the screen.
    * public Map getMap() : returns a Map if the preference must display a drop down box on the screen and the key are different from the values.
    * public boolean getCheckbox() : currently not implemented (i.e. always returns false).
    * public String getPreview() : returns a String that represents a preview of the preference.
    

In the RegisterServlet is enough to instantiate UserPreferencesUtil and pass it to the jsp. The jsp then

loops over the groups
  for each group loops over the preferences
    retrieves labels and additional content information where necessary

That's it.

If for some reason you need to call a preference directly (this is the case for the registration screen where new users are asked to choose a locale), it is as simple as

  <c:if test="${!empty userPreferences.groups['user.preferences.group.internationalization']['user.default.locale']}">
    <c:set var="locale" value="${userPreferences.groups['user.preferences.group.internationalization']['user.default.locale']}" />
    <div class="row">
      <label for="${locale.key}"><fmt:message key="${locale.label}" /></label>
      <span>
      <select name="${locale.key}" id="${locale.key}">
      <c:forEach items="${locale.map}" var="defaultLocale">
        <option value="<c:out value="${defaultLocale.key}" />"<c:if test="${newuser.preferences['user.default.locale'] == defaultLocale.key}"> selected="selected"</c:if>><c:out value="${defaultLocale.value}" /></option>
        </c:forEach>
      </select>
      </span> 
      <div class="formhelp"><fmt:message key="locale.help" /></div>
    </div>
  </c:if>

Bye -- Charles 26.07.2012 16:26 CEST

I can do the merge for you if you're encountering conflicts - just let me know. If you do it, first make sure you merge all recent changes from trunk to your feature branch, commit the merge from trunk, then merge your feature branch back to trunk - that should avoid most conflicts. I'll be flying tonight so probably won't have internet access, but the code review so far has looked good so I don't have any serious concerns about these changes going to trunk. Thanks again for all of your work! -- Ryan • (comments) • 26-Jul-2012 07:54 PDT

Ok. I will try it and, if necessary ask for your help. Thank you! -- Charles 27.07.2012 08:44 CEST

NullPointerException[edit]

I finished my tests. There is one minor issue, whose origin I cannot investigate right now (see below for details). I committed my changes to branch and will try to merge from trunk and see how it works quite soon. Testing the merge and merging back to trunk is probably going to be a task for the next days.

Tests:

  • Install and initialize a JAMWiki 1.2 instance
    • [√] Deploy JAMWiki 1.2 with HSQL
    • [√] Login as admin, add display name and signature
    • [√] Register a new user with locale != en and add display name
  • Upgrade to JAMWiki 1.3 with HSQL
    • [√] Remove tomcat deployment directory
    • [√] Empty tomcat cache
    • [√] Deploy 1.3
      • [√] Enter app details, start upgrade
      • [√] Click on "StartingPoints"
      • [x] Login as admin -> javax.servlet.jsp.JspException: java.lang.NullPointerException / java.lang.NullPointerException
      • [√] Click on Back
      • [√] (repeat test) Login as admin
      • [√] Click on Account: signature is there, including preview
      • [√] Logout and login as other user
      • [√] Click on Accoung: signature is there
      • [√] Change timezone and datetime format: Save succeeds
      • [√] Edit any topic and add signature: signature displays correctly (signature, datetime format and timezone).
  • Setup JAMWiki 1.3 from scratch wint HSQL
    • [√] Remove tomcat deployment directory
    • [√] Remove content from JAMWiki system directory
    • [√] Deploy JAMWiki 1.3
    • [√] Login as admin
    • [√] Click on Account: change signature
    • [√] Save succeeds: preview is available
    • [√] Edit any topic and test signature
    • [√] Register as a new user with a display name and a locale != en
    • [√] Click on Account: change timezone and datetime format
    • [√] Save succeeds
    • [√] Edit any topic and test signature
  • [√] Same set of tests with PostgreSQL: Everything ok (also no NullPointerException on login as admin, see above).

All tests have been repeated twice.

-- Charles 27.07.2012 09:50 CEST

Finally I found the time to make the merges, tests and commits. -- Charles 27.07.2012 12:10 CEST

I think I'll have some time to look into the admin login issue tomorrow. I also need to get a JAMWiki 1.2.2 release done, but life has been very busy recently... -- Ryan • (comments) • 28-Jul-2012 22:20 PDT
Unfortunately I only had time to commit some changes that I had queued locally today; I'll try to get to the admin login issue either tomorrow or Tuesday. -- Ryan • (comments) • 29-Jul-2012 18:58 PDT

I had a look at the tomcat log file. It seems that the application is unaware of the changes in the database. This may be a result of some caching mechanisms somewhere. It still puzzles me that the error does not appear while using PostgreSQL. Here are the messages (in a bit condensed form):

Jul 30, 2012 10:06:13 AM org.apache.catalina.startup.Catalina start
INFO: Server startup in 3124 ms
2012-07-30 10:06:53,622 [http-8080-2] ERROR o.j.a.JAMWikiAuthenticationProcessingFilter - Failure while updating last login date for charles
org.jamwiki.DataAccessException: org.apache.commons.dbcp.SQLNestedException: Borrow prepareStatement from pool failed 
  at org.jamwiki.db.AnsiDataHandler.lookupWikiUser(AnsiDataHandler.java:1328) ~[jamwiki-core-1.3-SNAPSHOT.jar:na]
  at org.jamwiki.db.AnsiDataHandler.lookupWikiUser(AnsiDataHandler.java:1348) ~[jamwiki-core-1.3-SNAPSHOT.jar:na]
  at org.jamwiki.authentication.JAMWikiAuthenticationProcessingFilter.successfulAuthentication(JAMWikiAuthenticationProcessingFilter.java:94) ~[jamwiki-web-1.3-SNAPSHOT.jar:na]
Caused by: org.apache.commons.dbcp.SQLNestedException: Borrow prepareStatement from pool failed
Caused by: java.sql.SQLException: user lacks privilege or object not found: JAM_USER_PREFERENCES_DEFAULTS
Caused by: org.hsqldb.HsqlException: user lacks privilege or object not found: JAM_USER_PREFERENCES_DEFAULTS
2012-07-30 10:07:23,357 [http-8080-2] ERROR o.j.a.JAMWikiAuthenticationProcessingFilter - Failure while updating last login date for charles
org.jamwiki.DataAccessException: java.sql.SQLException: statement is invalid
  at org.jamwiki.db.AnsiDataHandler.lookupWikiUser(AnsiDataHandler.java:1328) ~[jamwiki-core-1.3-SNAPSHOT.jar:na]
  at org.jamwiki.db.AnsiDataHandler.lookupWikiUser(AnsiDataHandler.java:1348) ~[jamwiki-core-1.3-SNAPSHOT.jar:na]
  at org.jamwiki.authentication.JAMWikiAuthenticationProcessingFilter.successfulAuthentication(JAMWikiAuthenticationProcessingFilter.java:94) ~[jamwiki-web-1.3-SNAPSHOT.jar:na]
Caused by: java.sql.SQLException: statement is invalid
Caused by: org.hsqldb.HsqlException: statement is invalid
2012-07-30 10:07:23,363 [http-8080-2] ERROR org.jamwiki.servlets.ServletUtil - Failure while retrieving user from database with login: charles
org.jamwiki.DataAccessException: java.sql.SQLException: statement is invalid
  at org.jamwiki.db.AnsiDataHandler.lookupWikiUser(AnsiDataHandler.java:1328) ~[jamwiki-core-1.3-SNAPSHOT.jar:na]
  at org.jamwiki.db.AnsiDataHandler.lookupWikiUser(AnsiDataHandler.java:1348) ~[jamwiki-core-1.3-SNAPSHOT.jar:na]
  at org.jamwiki.servlets.ServletUtil.currentWikiUser(ServletUtil.java:250) ~[jamwiki-web-1.3-SNAPSHOT.jar:na]
  at org.jamwiki.servlets.JAMWikiLocaleInterceptor.retrieveUserLocale(JAMWikiLocaleInterceptor.java:64) [jamwiki-web-1.3-SNAPSHOT.jar:na]
  at org.jamwiki.servlets.JAMWikiLocaleInterceptor.setUserLocale(JAMWikiLocaleInterceptor.java:76) [jamwiki-web-1.3-SNAPSHOT.jar:na]
  at org.jamwiki.servlets.JAMWikiLocaleInterceptor.preHandle(JAMWikiLocaleInterceptor.java:47) [jamwiki-web-1.3-SNAPSHOT.jar:na]
Caused by: java.sql.SQLException: statement is invalid
Caused by: org.hsqldb.HsqlException: statement is invalid
2012-07-30 10:07:23,373 [http-8080-2] ERROR org.jamwiki.servlets.ServletUtil - Failure while retrieving user from database with login: charles
org.jamwiki.DataAccessException: java.sql.SQLException: statement is invalid
  at org.jamwiki.db.AnsiDataHandler.lookupWikiUser(AnsiDataHandler.java:1328) ~[jamwiki-core-1.3-SNAPSHOT.jar:na]
  at org.jamwiki.db.AnsiDataHandler.lookupWikiUser(AnsiDataHandler.java:1348) ~[jamwiki-core-1.3-SNAPSHOT.jar:na]
  at org.jamwiki.servlets.ServletUtil.currentWikiUser(ServletUtil.java:250) [jamwiki-web-1.3-SNAPSHOT.jar:na]
  at org.jamwiki.servlets.ServletUtil.topicParserInput(ServletUtil.java:699) [jamwiki-web-1.3-SNAPSHOT.jar:na]
  at org.jamwiki.servlets.ServletUtil.viewTopic(ServletUtil.java:852) [jamwiki-web-1.3-SNAPSHOT.jar:na]
  at org.jamwiki.servlets.TopicServlet.view(TopicServlet.java:75) [jamwiki-web-1.3-SNAPSHOT.jar:na]
  at org.jamwiki.servlets.TopicServlet.handleJAMWikiRequest(TopicServlet.java:47) [jamwiki-web-1.3-SNAPSHOT.jar:na]
  at org.jamwiki.servlets.JAMWikiServlet.handleRequestInternal(JAMWikiServlet.java:331) [jamwiki-web-1.3-SNAPSHOT.jar:na]
Caused by: java.sql.SQLException: statement is invalid
Caused by: org.hsqldb.HsqlException: statement is invalid
2012-07-30 10:07:23,392 [http-8080-2] ERROR org.jamwiki.servlets.ServletUtil - Failure while retrieving user from database with login: charles
org.jamwiki.DataAccessException: java.sql.SQLException: statement is invalid
  at org.jamwiki.db.AnsiDataHandler.lookupWikiUser(AnsiDataHandler.java:1328) ~[jamwiki-core-1.3-SNAPSHOT.jar:na]
  at org.jamwiki.db.AnsiDataHandler.lookupWikiUser(AnsiDataHandler.java:1348) ~[jamwiki-core-1.3-SNAPSHOT.jar:na]
  at org.jamwiki.servlets.ServletUtil.currentWikiUser(ServletUtil.java:250) ~[jamwiki-web-1.3-SNAPSHOT.jar:na]
  at org.jamwiki.servlets.JAMWikiServlet.buildUserMenu(JAMWikiServlet.java:235) [jamwiki-web-1.3-SNAPSHOT.jar:na]
  at org.jamwiki.servlets.JAMWikiServlet.loadLayout(JAMWikiServlet.java:428) [jamwiki-web-1.3-SNAPSHOT.jar:na]
  at org.jamwiki.servlets.JAMWikiServlet.handleRequestInternal(JAMWikiServlet.java:334) [jamwiki-web-1.3-SNAPSHOT.jar:na]
Caused by: java.sql.SQLException: statement is invalid
Caused by: org.hsqldb.HsqlException: statement is invalid
2012-07-30 10:07:23,397 [http-8080-2] ERROR org.jamwiki.servlets.ServletUtil - Failure while retrieving user from database with login: charles
org.jamwiki.DataAccessException: java.sql.SQLException: statement is invalid
  at org.jamwiki.db.AnsiDataHandler.lookupWikiUser(AnsiDataHandler.java:1328) ~[jamwiki-core-1.3-SNAPSHOT.jar:na]
  at org.jamwiki.db.AnsiDataHandler.lookupWikiUser(AnsiDataHandler.java:1348) ~[jamwiki-core-1.3-SNAPSHOT.jar:na]
  at org.jamwiki.servlets.ServletUtil.currentWikiUser(ServletUtil.java:250) [jamwiki-web-1.3-SNAPSHOT.jar:na]
  at org.jamwiki.servlets.ServletUtil.currentWatchlist(ServletUtil.java:290) [jamwiki-web-1.3-SNAPSHOT.jar:na]
  at org.jamwiki.servlets.JAMWikiServlet.buildTabMenu(JAMWikiServlet.java:175) [jamwiki-web-1.3-SNAPSHOT.jar:na]
  at org.jamwiki.servlets.JAMWikiServlet.loadLayout(JAMWikiServlet.java:429) [jamwiki-web-1.3-SNAPSHOT.jar:na]
  at org.jamwiki.servlets.JAMWikiServlet.handleRequestInternal(JAMWikiServlet.java:334) [jamwiki-web-1.3-SNAPSHOT.jar:na]
Caused by: java.sql.SQLException: statement is invalid
Caused by: org.hsqldb.HsqlException: statement is invalid
2012-07-30 10:07:23,549 [http-8080-2] ERROR org.jamwiki.jsp - Error in JSP page
javax.servlet.jsp.JspException: java.lang.NullPointerException
  at org.apache.jsp.tag.web.wikiMessage_tag.doTag(wikiMessage_tag.java:107) ~[na:na]
Caused by: java.lang.NullPointerException: null
  at org.apache.jasper.runtime.BodyContentImpl.write(BodyContentImpl.java:156) ~[jasper-6.0.24.jar:na]
  at org.apache.jsp.tag.web.wikiMessage_tag._jspx_meth_c_005fotherwise_005f0(wikiMessage_tag.java:316) ~[na:na]
  at org.apache.jsp.tag.web.wikiMessage_tag._jspx_meth_c_005fchoose_005f0(wikiMessage_tag.java:244) ~[na:na]
  at org.apache.jsp.tag.web.wikiMessage_tag._jspx_meth_c_005fif_005f0(wikiMessage_tag.java:217) ~[na:na]
  at org.apache.jsp.tag.web.wikiMessage_tag._jspx_meth_fmt_005fparam_005f0(wikiMessage_tag.java:187) ~[na:na]
  at org.apache.jsp.tag.web.wikiMessage_tag._jspx_meth_fmt_005fmessage_005f0(wikiMessage_tag.java:148) ~[na:na]
  at org.apache.jsp.tag.web.wikiMessage_tag.doTag(wikiMessage_tag.java:96) ~[na:na]
javax.servlet.jsp.JspException: java.lang.NullPointerException
  at org.apache.jsp.tag.web.wikiMessage_tag.doTag(wikiMessage_tag.java:107)
Caused by: java.lang.NullPointerException

Wednesday this week will be the national holiday of Switzerland. Then I could sure find some time to try to get closer to the origin of the problem. So if your schedule is too tight, let me know -- Charles 30.07.2012 12:53 CEST

The problem was really the cache. I added now a new method to DataHandler and its implementation to delete all caches after an upgrade. Additionally I make a call to WikiDatabase.initialize(). This solves the problem. If it needs to be that radical is an open question. As a matter of fact after an upgrade there could always be some changes in the database that require resetting the system. I made tests with both HSQL and PostgreSQL. It is still uclear to me, why the upgrade with PostgreSQL never showed the problem. I committed the changes to trunk and you are welcome to check the changes. Bye -- Charles 30.07.2012 19:48 CEST

I slightly modified your commit in revision 4131 just to avoid increasing the DataHandler further - that interface is already fairly bloated. The functionality should be the same, but please let me know if you see any issues. -- Ryan • (comments) • 30-Jul-2012 21:46 PDT

The modified code clears the cache but does not reinitialize the SQL statements. Testing I had the NullPointerException again. I added the code line WikiDatabase.initialize() right before your call to WikiCache.initialize() in UpgradeServlet and tested again. It works with HSQL and PostgreSQL. So I think that now the code is ready. If I find more time I may take a look at the TOC preference. -- Charles 31.07.2012 09:57 CEST

I'll take a closer look at this - WikiDatabase.initialize() should just refresh the connection pool, which I don't think should be required, so it's possible there is another bug hidden in there somewhere. Thanks for reviewing the change. -- Ryan • (comments) • 31-Jul-2012 07:58 PDT

Weird. Well, I repeated the upgrade scenarios and found the following.

1. The log files for 1.2 -> 1.3 without WikiDatabase.initialize() with PostgreSQL and with WikiDatabase.initialize() HSQL and PostgreSQL are identical in terms of exception listing. All three have a ERROR logged, which is normal during the upgrade process:

During the upgrade process the admin user is required to log in. At this point in time, the software already uses the new database schema for WikiUser and its preferences, but the database has not been modified yet. The exception is caught and ignored because it is not fatal. It happens where the last login date should be updated in the database.

2012-08-02 08:44:05,504 [http-8080-1] ERROR o.j.a.JAMWikiAuthenticationProcessingFilter - Failure while updating last login date for charles
org.jamwiki.DataAccessException: org.postgresql.util.PSQLException: ERROR: relation "jam_user_preferences_defaults" does not exist

2. The log file for the HSQL case without the WikiDatabase.initialize() contains additional exceptions that seem to be related to the statements loaded in the DataHandler implementations:

2012-08-02 08:22:21,118 [http-8080-2] ERROR org.jamwiki.servlets.ServletUtil - Failure while retrieving user from database with login: charles
org.jamwiki.DataAccessException: java.sql.SQLException: statement is invalid

After restarting tomcat, the statements should be correct, because they are reloaded from the properties file. So I am a bit confused. What I would try is to let the app write the statement it is using to the log file and see if it gives a clue what is happening.

I am not sure that I can look at that today. If so I would post a note here. And let me know if you make some progress -- Charles 02.08.2012 09:53 CEST

I think I found the problem - there were some statements not closed in finally blocks, so if an exception was thrown then the statement was left open. After tonight's commits I no longer see exceptions during upgrade, but please let me know if you see any further problems. -- Ryan • (comments) • 02-Aug-2012 20:03 PDT

I made an update of my working trunk, compiled and deployed as usual (first 1.2 then 1.3). I still had the NullPointerException. Just to make sure I deleted my working trunk completely and repeated the process: No exception. I will make some more testing during the day to see if the exception does not appear anymore and report the results. I will also have a look at the changes in the code. I thought I had closed all statements, since we had this problem once already a few weeks ago... But again, that is the purpose of reviewing code :-) Thank you for looking at that -- Charles 03.08.2012 07:33 CEST

The statements were all closed, but since some of them weren't closed in finally blocks it looked to me like there was potential for leaving some open. I also found a potential database connection leak and have queued that fix for 1.2.2. -- Ryan • (comments) • 02-Aug-2012 22:42 PDT

Still strange. I still get the NullPointerException. I made some modifications to the AnsiDataHandler class. The lookupWikiUser does not need a transaction, it's only selects. Adding a DatabaseConnection.closeConnection() to writeWikiUser solved the problem. I guess, however, that this is the same effect as resetting the connection pool, only limited to a single connection. In that sense I am not sure if there is not still some problem somewhere.

The most intriguing part of it is, that after a fresh checkout and compile everything worked fine. I tested it twice, i.e. checkout and compile, without both steps I get the exception. All other steps are absolutely the same. For that reason I discarded the hypothesis that the origin of the problem might be in my local environment. I packed the deployment steps into two script files to make sure that they are always the same.

Well I am still puzzled by the problem. Could that be that HSQL jdbc implementation does not close statements correctly? And am I correct assuming that you don't get the exception if you go through the upgrade process a second time?

I will commit my changes to my branches/cclavadetscher. As long as it is not clear if this solution is good enough I prefer not to commit to trunk. -- Charles 03.08.2012 10:54 CEST

Maybe the connection pool has a problem: look at this: http://sourceforge.net/projects/hsqldb/forums/forum/73674/topic/4587597. Can you make something useful out of it? -- Charles 03.08.2012 12:23 CEST

Thanks for continuing to follow up. I'll try to look into this further during the weekend - the errors I was seeing were fixed by the changes I committed, but obviously something is still broken. It would probably be better not to change writeWikiUser since it does need a transaction, but the fact that changing things fixes the issue for you may help in tracking down the problem. Are you still seeing the same stack trace as before with the latest commits? -- Ryan • (comments) • 03-Aug-2012 07:56 PDT

Yes, exactly the same stack trace. I uploaded a copy unter tomcat.log. There is a small misunderstanding :-) The modifications I made to writeWikiUser did not include removing the transaction code. It is clear that write ops and affecting two tables need a transaction. What I did there, is merely move the code lines with the commit statement and the update of the cache into the try section and add the connection close line in the (new) finally section. I read somewhere that this makes code cleaner. I removed the transaction code from lookupWiSerkiUser, since it's only reading ops and there is no danger of ending up with an inconsistent status in the database. It is a fact, too, that closing the connection in writeWikiUser solves the issue. So there must be a relationship between the connection and the exception. So far I still don't see it, but according to the many contribs on the link I posted previously, the pool seems not be completely safe, depending on the version and configuration. Well, let's keep in touch. I hope this does not create more trouble than necessary. -- Charles 03.08.2012 18:20 CEST

I think I was able to reproduce your error after clearing cookies - I think the critical step is that writeWikiUser must fail after login. If I'm reading the bug you reported above correctly then it does sound like flushing the connection pool might be required, since HSQL isn't correctly handling a schema change when pooled statements are being used. As a result, re-adding the WikiDatabase.initialize() call with a comment that pooled statements must be flushed after a schema change seems like it is probably the right solution.
Also, apologies for the mis-understanding of your transaction change - I usually check recent changes immediately after waking up, and hadn't had my coffee yet :) Removing a transaction from lookup code makes sense, and making sure everything is closed in a finally block is a good idea as well. Thanks for your persistence in tracking down these issues. -- Ryan • (comments) • 05-Aug-2012 20:09 PDT

Since in earlier posts you wrote that you did not see the problem I was not sure that you may be able to analyze the problem and was thinking of spending some time today with that. Needless to say that I am quite relieved that you could reproduce the exception situation. It is not a good feeling if the same codebase behaves differently in identical situations. It remembers me of the old C-programming times... :-) As a good measure I clean up the tomcat cache during each deployment, just to avoid side effects.

I agree with you that reinitializing the database pool is a good idea (better than the second solution, i.e. closing the connection at the end of writeWikiUser). So far we both could not see any other potential reasons for the exception that could lead to issues during normal operations. The assumption that the issue is external to the JAMWiki codebase is therefore legitimate. I will make the changes and add the comments. Most of it is already done. What I need to do is to remove the connection closing statement from writeWikiUser, which is not necessary and re-add the WikiDatabase.initialize in UpgradeServlet along with the comment. I will keep the modifications I made to lookupWikiUser (no transaction), merge and commit. Wish you a good start in the new week -- Charles 06.08.2012 06:46 CEST

I forgot a point: No need for apologies, but, honestly, to go online before a cup of good strong coffee is almost "criminal". So if you didn't have your coffee yet, then go for it :-) Enjoy -- Charles 06.08.2012 07:03 CEST - I just noticed that the timestamp was wrong... The previous one was computed correctly. Weird. Well this is another problem.

The changes are commited to trunk -- Charles 06.08.2012 09:36 CEST

I made a slight change to your commit - upgradeDatabase is invoked first to see if database changes are needed, and then again to actually perform the changes, so I moved the cache clear and pool re-initialization into that method - the only downside I can see to that approach is that they may be invoked more than is necessary, which shouldn't hurt anything. As always, thanks for your work.
Between now and the end of September I'm going to be spending a lot of time on planes and in airports, which is when I tend to get a lot of coding done, so hopefully the every-six-month-major-release-cycle can continue with JAMWiki 1.3 seeing an October release (JAMWiki 1.2 launched in late March). -- Ryan • (comments) • 06-Aug-2012 21:50 PDT

Updates[edit]

I made a couple of minor changes:

  1. revision 4206 uses the time zone preference for all JSPs where date is displayed, rather than just in the user signature.
  2. revision 4204 sorts the time zones alphabetically on the user preferences UI to make them easier to find.

I'd also like to modify the date preferences so that they are used in JSPs that display date, although to do so may require splitting the "date format" and "time format" preferences - I'm still investigating whether there is an alternative. -- Ryan • (comments) • 02-Dec-2012 10:45 PST

revision 4213 splits the datetime preference into two preferences for date and time. To update any JAMWiki instance that was running with the old preferences (ie if you were running JAMWiki 1.3 development code) execute the following SQL to update your system:

delete from jam_user_preferences where pref_key = 'user.datetime.format';
delete from jam_user_preferences_defaults where pref_key = 'user.datetime.format';
insert into jam_user_preferences_defaults VALUES('user.date.format', NULL, 'user.preferences.group.internationalization', 3);
insert into jam_user_preferences_defaults VALUES('user.time.format', NULL, 'user.preferences.group.internationalization', 4);
commit;

-- Ryan • (comments) • 16:46, 06 December 2012 (PST)

Currently the drop down box with the list of locales that the user can choose from includes all locales available on the system, regardless if there is a resource file for that language or not. Would it make sense to modify this behaviour to use only the available translations? I have seen an entry in jamwiki-configuration-1.3.xml that could be used for that. Bye -- Charles 01:50, 29 December 2012 (PST)

I had a look at the point above and it was a very simple change that I already did and committed. The necessary methods to handle the list of translations was already available in the configuration file. Let me know if you see any drawback in this approach and I will revert the change. Bye -- Charles 02:13, 29 December 2012 (PST)

I think there are three arguments in favor of showing all available locales:
  1. Any code (such as date formatting) that uses locale would benefit from having the correct locale.
  2. If translations are later added for a locale a user won't have to change their preferences - translations will simply appear.
  3. Showing all system locales is the legacy behavior, and a change might confuse some users.
The argument in favor of showing only those locales for which translations are available is that it is less confusing to users, and that's a strong argument. However, we might be able to reduce confusion by changing the help text to "The specified locale is used to determine how labels and other text on the wiki should be displayed. Note that if translations are not available for a locale then labels will still display in English." Would that make sense?
Also, for clarification, the entries in the jamwiki-configuration-1.3.xml were originally added when the si_LK and gl translations were added since many systems don't have those locales available. Currently the code combines the system locales with the configured locales to produce a final list. -- Ryan • (comments) • 08:16, 31 December 2012 (PST)

I find the arguments absolutely valid. The idea of modifying the text as you suggest would greatly reduce the potential for user confusion. I made the revert changes to the code already and committed them to trunk. I will do the changes to the text tomorrow, unless you find time for that. In less than two hours time (we have now a few minutes past 22:00) is end of year and my girl friend is becoming a bit nervous of my sitting in front of the screen :-) So, as mentioned, let me know if you do the change to the text. If not, I will do that tomorrow. Bye -- Charles 13:09, 31 December 2012 (PST)

revision 4237 includes the label text update. Enjoy the new year - I just got done with my yearly end-of-year camping trip. My plan right now is to review code tomorrow with the hope of getting the 1.3 beta out ASAP. -- Ryan • (comments) • 15:58, 31 December 2012 (PST)

Great. I will then just update the German and Italian versions of the text. Maybe it would be worth to inform potential translators that that particular text was modified, so that new versions on other languages also show the change. As for the review, I think that the code (in the dialog betweeen servlet and jsp) works ok, but could be improved in the future for better maintenance. If I find some time I will sketch a sequence diagram to find out a nicer solution. But, as I said so long, it works, but may feel a bit cryptical, although I tried to point out in comments, what was the rationale behind some decision. Bye, and enjoy the new year, too. I think that there are some very interesting and challenging things coming up -- Charles 17:14, 31 December 2012 (PST)

I made a number of updates to the user preference management code tonight but didn't look at the email code very closely yet. I'm anxious to get the beta release out, so if I haven't had a chance to review the email code in the next couple of days I may just push the first beta release as-is, and then any required fixes can be made for beta 2 or 3. If you see anything that needs fixing or cleaning up please feel free to commit the change directly to trunk. -- Ryan • (comments) • 21:38, 01 January 2013 (PST)
Also, in response to your question about translations, usually if a key changes in English I just remove it from the other language translation files, that way the translators for those languages will know it needs to be re-translated. That's a bit crude, but thus far no one has suggested a better alternative. -- Ryan • (comments) • 21:44, 01 January 2013 (PST)
One final note, I just pushed the latest code to jamwiki.org, so even though email isn't yet enabled here the latest code is running on this server. -- Ryan • (comments) • 21:55, 01 January 2013 (PST)

On Tech:Email I added instructions on how to use a gmail account from the wiki. If you think that you can use it for jamwiki.org I would suggest to remove the password from the wiki, just to avoid abuse. To see the reset password in action you may go to my private page and register. For using the feature you don't need write access -- Charles 01:40, 02 January 2013 (PST)