Allow name of session and moqui.visitor cookie to be configured. moqu…

[shendepu]

…i.visitor cookie name can be changed by system property moqui_visitor_cookie_name, while session cookie name JSESSIONID can be configured by webapp.session-config.@cookie-name in MoquiConf.xml

[chunlinyao]

I think session cookie name should be managed by the java EE container, instead of moqui itself.
UPDATE:
Sorry, I misunderstanded your patch, you are using the config api.

[shendepu]

@jonesde Hi David, do you have any comment on this PR?

[jonesde]

I will sooner or later, have bigger fish to fry (more significant issues to handle).

From a quick read through this is wrong (in UserFacadeImpl):

visitorCookieName = System.getProperty("moqui_visitor_cookie_name")

To use a system property through the Moqui Conf XML file there should be a placeholder in the file with the property name, and the code should get the value from the MNode that came from the parsing the XML file and expanding placeholders (in ${}) against system or default property settings.

[shendepu]

Sure, no problem when you have time on reviewing this.

I have moqui_visitor_cookie_name defined with default value in MoquiDefaultCon.xml (https://github.com/shendepu/moqui-framework/blob/0f7331daaeabe91123bc5155e4588d7b1921f24e/framework/src/main/resources/MoquiDefaultConf.xml#L22). So we may change it in MoquiConf.xml in our component if need to rename cookie moqui.visitor.

[jonesde]

Just looked at this again and in UserFacadeImpl it still gets the cookie name directly from a system property which breaks the pattern of being able to set it in the Moqui Conf XML file (which can in turn get it from the system property), ie this on line 74 of your patched version:

visitorCookieName = System.getProperty("moqui_visitor_cookie_name")

This should be changed to look at the webapp.session-config.@cookie-name attribute that you added.

In the MoquiDefaultConf.xml file instead of this:

<session-config timeout="60" cookie-name="JSESSIONID"/>

you would need this to pick up the system property or JVM param:

<session-config timeout="60" cookie-name="${moqui_visitor_cookie_name}"/>

[jonesde]

Wait, looking at this again it looks like you're actually trying to do 2 different cookie names, the session cookie name and the visitorId cookie name. Both should be handled the same way, ie following the pattern of what I commented on last but we'll need 2 attributes in the Moqui Conf XML file and 2 property names to expand from.

It would be better to name these more distinctly too, ie 'session-cookie-name' along with moqui_session_cookie_name and 'visitor-cookie-name' along with moqui_visitor_cookie_name.

[shendepu]

I understand the comments, here is my thought:

1. session cookie name is handled differently with visitor-cookie-name.
   There is already a webapp.session-config.@timeout in conf to change session config, and session cookie name is related to session config, so I handle it here.

2. use System.getProperty()
   I recalled that Moqui merges property in moqui-conf and environment variable that environment variable overrieds moqui-conf property . So the cookie name can be set in environment variables without changing the conf file.

[shendepu]

for comment

It would be better to name these more distinctly too, ie 'session-cookie-name' along with moqui_session_cookie_name and 'visitor-cookie-name' along with moqui_visitor_cookie_name.

for visitor cookie, it is already named moqui_visitor_cookie_name.
for session cookie I should also add a property moqui_visitor_cookie_name and expand it in session-config.@cookie-name

[jonesde]

Why make it more complicated by using different patterns for configuring different things? All it does is make configuration for users more difficult, and documentation look odd because to properly explain how to do this and that it would have to clarify the differences.

IMO the moqui_ prefix should be removed as it also breaks the pattern used with other properties. I'm not so worried about property collisions (they I often do a quick google search on candidate property names for potential conflicts). Usually System properties are used only when Moqui is deployed in something like a container where it is the only thing running, and for more custom bare metal deployments usually Java env vars or actual Moqui Conf XML files are more convenient.

[shendepu]

OK. I will make a change that remove moqui- prefix and just config session cookie name via session_cookie_name in moqui-conf property

BTW, is it right that I use System.getProperty('visitor_cookie-name') as environment variable overrides moqui conf property

[shendepu]

And I name it as visitor_cookie_name instead of visitor-cookie-name since it is more common to use _ than - in environment variable name