Hi Ilya, Elena.
Sorry for delay with patches evaluation; I agree that Harmony implementation is not compatible with RI and we should fix it. The problems I see here are that the class behavior is OS-depended, not explicitly specified (we don't know how SPI methods were implemented by RI) and hard to test the class implementation (we depend on backing store's state). I was going to fix patches myself but I've realized that there are a lot of changes and it'd better to discuss them first.
1) Let's review the patch for RegistryPreferencesImpl.java first - it fixes the next methods:
- childSpi(String name);
- putSpi(String name, String value),
The patch suggests ignoring error code returned by native API and printing a warning instead of throwing SecurityException. I disagree with this approach.
The spec. for AbstractPreferences class says: "The remaining SPI methods putSpi(String,String), removeSpi(String) and childSpi(String) have more complicated exception behavior. They are not specified to throw BackingStoreException, as they can generally obey their contracts even if the backing store is unavailable ... There is one circumstance under which putSpi, removeSpi and childSpi should throw an exception: if the caller lacks sufficient privileges on the underlying operating system to perform the requested operation. This will, for instance, occur on most systems if a non-privileged user attempts to modify system preferences..."
So we should throw SecurityException if error code is ERROR_ACCESS_DENIED.
For example, you may wish to try the following test with RI:
- open registry with registry editor
- find HKEY_LOCAL_MACHINE / SOFTWARE / JavaSoft / Prefs entry
- deny all permission for you
- run a test that only invokes: Preferences.systemNodeForPackage(Object.class);
RI will throw SecurityException. And Harmony with the suggested patch applied will print a warning only.
From my point of view a native code should be fixed first. For example, childSpi(String) methods invokes getNode() that has the following algorithm:
- invoke openRegKey() that in turn tries to open registry key and if it fails to open the key it creates it.
- checks error code returned by openRegKey() and if err!=ERROR_SUCCESS returns
- the next step is confusing for me: so we have err==ERROR_SUCCESS now but the method invokes RegCreateKeyEx again and returns dwDisposition==REG_CREATED_NEW_KEY value (that always is false in this case)
I'd fix it in the next way: first try to create a registry key and check returned error code. If the err== ERROR_ACCESS_DENIED just simply verify if the key exists (by invoking RegOpenKeyEx). Otherwise return the error code.
A minor note: RI uses logging API for printing warnings instead of System.out. Frankly, I'm now quite sure what we should use.
2) Now the patch for the test.
I agree that behavior depends on backing store's state so BackingStoreException can be thrown. (BTW, I'd also catch SecurityException for systemNodeForPackage(Object.class) see example above)
But I was thinking how to make the testing more predictable (I like automated testing and dislike to edit registry keys by hands). For example, just crazy idea, to develop a native library for testing that let the test verify registry permissions. So the test can check permissions for registry keys first and them it can verify that a method does the right things. Is this possible?
A minor note: I'd pass to systemNodeForPackage() PreferencesTest.class instead of Object.class. So if the test fails to remove created key it is clear who put this garbage to the registry.