Input Sanitation in AppSettings#save()

[MeFisto94]

For reference see jMonkeyEngine/wiki#97, TLDR: essentially dots have to be replaced with forward slashes and maybe an overloaded Version which accepts class could be made, which would be more following the specification.

[louhy]

Since I raised the original, I can take a look at this. Initial thoughts:

1) Also need a load() in addition to save() which accepts Class instead of 'String preferencesKey' 2) Deprecate the old String based methods with instruction to use the new Class methods instead 3) Could also add a clearSavedPreferences(Class mainClass) method which calls Preferences.clear() to support "forgetting" saved settings 4) For the deprecated String methods, replace "." with "/" 5) For the deprecated String methods, could also toLower() those, but since that changes current behavior further, not sure. Thoughts? (4. also changes behavior... need to look up the policy we decided on for breaking changes. Might be better to just skip 4 & 5.)

New method signatures would look something like (parameter name suggestions welcome):

public void load(Class mainClass) throws BackingStoreException;
public void save(Class mainClass) throws BackingStoreException;
public void clearSavedPreferences(Class mainClass) throws BackingStoreException;//Useful?

[louhy]

6. SettingsDialog and TestCustomAppSettings call save(String preferencesKey) (and load) so we'd want to also update those to use these new Class based methods, if at all possible... possibly in a separate/later PR.

Glancing at that I'm not sure it'll be as simple of a change to make. There isn't a Class readily available to pass in the SettingsDialog case, so it'd require more extensive changes.

[riccardobl]

This looks a bit over engineered. I would just replace "/" with ".". If we really want to provide backward compatibility with the broken method, we can have a try catch in load() that inside the catch attempts to load without repacing "/" , so that it will still work if someone is trying to load preferences saved with a version before this fix.

[louhy]

I'd be fine with whatever, I'd just like it to work correctly with a readable folder path. Certainly less work to do it that way.

[pspeed42]

So this isn't really to fix any sort of bug... just to have a human-readable folder path?

I just want to make sure we're all clear on what we're trying to "fix" here.

[louhy]

Original intent started out as just a documentation update so that what the docs say should happen, actually happened. As we drilled down further, some felt the behavior could be improved to make things more reliable, and I agreed.

Settings should be stored in a specific folder path, but are instead put in a folder like this:

"HOME/.java/.userPrefs/_!'o!^@"v!'4!aw"l!(k![@"u!'c!a@"u!'!)!!z!#4!:w!t!(!!a!"5!(:!a@"j!(:!.@"0!'!cw"0!(:!.@!2!$g!;g!5"

Given that this isn't what's supposed to happen, harmless or not, I'd call it a bug.

Also since that crazy string varies in length, theoretically it could exceed a file system's maximum, or maybe even try to use a character not allowed in folder names. It may have been preventing saved settings from loading in certain situations. IIRC a lack of saving settings was annoying me during test development working with sgold; he mentioned they should save, so I looked into it, and found this.

[pspeed42]

Hmmm... I learned a long time ago never to mess around in the .java/.userPrefs folder so I never even thought about people looking there. The file paths are often an "implementation detail" and you may fix it today to have it break again for some other reason in the future.

Presumably, it is the Java preferences API generating that string and not us.

re: "Settings should be stored in a specific folder path" ...then maybe we shouldn't be using a black-box API to store them if we care about where and how they are stored.

[louhy]

JDK code is generating the string, yes. I took a look at the JDK code for this stuff. This crazy folder name is intended behavior to cope with invalid characters. Since I was only going to do a documentation fix originally, it wasn't investigated deep enough.

I find myself leaning more towards leaving the code as-is, and calling this a "documentation issue" like I did originally. At least I learned more about how the JME settings stuff works. :)

To be clear, I don't care how or where this stuff is stored. These folder names just look wrong. Generating those things certainly isn't how I'd have implemented it.

[MeFisto94]

So this isn't really to fix any sort of bug... just to have a human-readable folder path?

No. I don't have the docs at hand but at one point the javadoc clearly pointed out that one MUST NOT use dots and save() just doesn't care, hence the easy solution replacing them with slashes to not break that contract.

Everything else indeed is to improve usability (Passing a class instead of this.class.getCanoncialName()), because classes often are used for that UUID.

[louhy]

If the JDK docs do say that (then that API sucks), and maybe that character replacement is worth doing. The other changes above using class, I don't know.

It's worth noting that when I run an example from jme3-examples, it passes the name of the window as the preferencesKey, which by default contains a period:

jMonkeyEngine 3.3-6878

[MeFisto94]

I told it the wrong way, we must not use forward slashes but dots. Source: https://docs.oracle.com/javase/7/docs/api/java/util/prefs/Preferences.html

Every other node has an arbitrary node name, specified at the time it is created. The only restrictions on this name are that it cannot be the empty string, and it cannot contain the slash character ('/').

Then there also is https://docs.oracle.com/javase/8/docs/api/java/util/prefs/Preferences.html#userNodeForPackage-java.lang.Class- which states

Returns the preference node from the calling user's preference tree that is associated (by convention) with the specified class's package. The convention is as follows: the absolute path name of the node is the fully qualified package name, preceded by a slash ('/'), and with each period ('.') replaced by a slash. For example the absolute path name of the node associated with the class com.acme.widget.Foo is /com/acme/widget.

Now I am a bit confused, which one of the both is correct? I guess one must not use / because that defines a sub-level, but the forClass method does replace dots with slashes because it defines some sort of tree-hiearchy?

[pspeed42]

The first is talking about a node. It's like a File. File cannot have a path separator in its name. A node cannot have a path separators in its name.

The second part is talking about building out the hierarchy for a node. Sort of like creating a file with a PATH. The path to that node can contain path separators because split up those will make up the node names of the hierarchy. But none of the nodes in that hierarchy PATH will have a separator in their names.