Add PMD rules for default locale, charset and time zone

[Nadahar]

When I work with OH code, I see reliance on default locale, charset or time zone from time to time. I can't pursue that every time, because I would never get to do what I originally intended to do. But, it is my impression, both from OH and from other projects, that there are many that aren't aware of the pitfalls of relying on these defaults.

They are all JVM wide. The default Locale and TimeZone can be set by any code running on the JVM at any time. It wasn't long ago where we found a case where GraalPy seems to set the JVM-wide time zone if you invoke certain Python commands. Any add-on, official or not, can do the same. So, in essence, these defaults can change at any time, and you have no reason to expect them to be what you want. They are also affected, by default, by an incorrectly configured system or java command-line arguments. Charset has no method to set the default, but it can still be set by setting a system property or the COMPAT flag. Prior to Java 18, it was also OS dependent, since Java 18 it is UTF-8 by default, but that doesn't mean that it's safe to assume that it is UTF-8.

It's thus almost never "safe" or desirable to use these defaults. There are other properties one can use if one wants to get the "native"/OS defaults, which should be used when that is the need. Except for that, it's almost always better to use an explicit value.

I discovered that PMD 7.17.0 includes a new standard rule, RelianceOnDefaultCharset, which detects use of the default Charset, but I could find no solution for the other two.

I've therefore downloaded the OpenJDK source code and grep'ed my way through it in search of methods that implicitly use the default locale or time zone. This isn't an easy task, some calls are convoluted, and it's not always easy to predict when they come into play. I've also excluded some things that seem completely irrelevant, like AWT or Swing methods. But, I believe that I've been able to catch the most common pitfalls.

Use of the default values will only generate warnings. Trying to set the defaults will generate errors. I don't know if there's any place in the code where these should be set, but if there is, I think they should rather be handled with suppressions, because that's normally a really bad thing to do (given that many things rely on them even though they shouldn't).

Fixes #496.

Note: Writing custom PMD rules is new to me, so I might have made some rookie mistakes.

[Copilot]

Pull request overview

This PR adds PMD rules to detect and warn about reliance on default locale, charset, and time zone in Java code. The changes upgrade PMD from 7.16.0 to 7.17.0 to leverage a new built-in rule for charset detection and introduce four custom PMD rules for locale and time zone checks.

• Upgrade PMD version from 7.16.0 to 7.17.0
• Add RelianceOnDefaultCharset rule to detect default charset usage
• Implement four custom PMD rules to detect implicit and explicit usage of default locale and time zone

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
pom.xml	Updates PMD dependency version to 7.17.0
sat-plugin/src/main/java/org/openhab/tools/analysis/tools/PmdChecker.java	Updates PMD version constant to 7.17.0
sat-plugin/src/main/resources/rulesets/pmd/rules.xml	Enables the built-in RelianceOnDefaultCharset rule
sat-plugin/src/main/resources/rulesets/pmd/customrules.xml	Registers four new custom rules for locale and time zone checks
custom-checks/pmd/src/main/java/org/openhab/tools/analysis/pmd/SetDefaultLocale.java	New rule to detect attempts to set the default locale
custom-checks/pmd/src/main/java/org/openhab/tools/analysis/pmd/SetDefaultTimeZone.java	New rule to detect attempts to set the default time zone
custom-checks/pmd/src/main/java/org/openhab/tools/analysis/pmd/ImplicitDefaultLocaleRule.java	New rule to detect methods that implicitly use the default locale
custom-checks/pmd/src/main/java/org/openhab/tools/analysis/pmd/ImplicitDefaultTimeZoneRule.java	New rule to detect methods that implicitly use the default time zone

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[kaikreuzer]

Many thanks @Nadahar for your detailed description and the subsequent PR for it! I agree that those defaults are a regular cause of unexpected issues and that the awareness among developers is not very high. So I appreciate having such checks done here!
As soon as you address the two Copilot comments, we should be good to merge!

[Nadahar]

I'm chasing down a bug in the Astro binding at the moment, so I'll finish that before addressing the review. I assume a few hours (hopefully) won't matter here.

[Nadahar]

I've addressed the review comments and rebased on latest main.

[Nadahar]

A CheckStyle test fails because the result lacks Number:VolumePrice. I can't see how that can be related to this PR though.

FYI:
testInvalidItemType() fails because this is returned:

15: Value Invalid is not facet-valid with respect to enumeration [Call, Color, Contact, DateTime, Dimmer, Group, Image, Location, Number, Number:Acceleration, Number:AmountOfSubstance, Number:Angle, Number:Area, Number:ArealDensity, Number:CatalyticActivity, Number:Currency, Number:DataAmount, Number:DataTransferRate, Number:Density, Number:Dimensionless, Number:ElectricCapacitance, Number:ElectricCharge, Number:ElectricConductance, Number:ElectricConductivity, Number:ElectricCurrent, Number:ElectricInductance, Number:ElectricPotential, Number:ElectricResistance, Number:EmissionIntensity, Number:Energy, Number:EnergyPrice, Number:Force, Number:Frequency, Number:Illuminance, Number:Intensity, Number:Length, Number:LuminousFlux, Number:LuminousIntensity, Number:MagneticFlux, Number:MagneticFluxDensity, Number:Mass, Number:Power, Number:Pressure, Number:RadiationDoseAbsorbed, Number:RadiationDoseEffective, Number:RadiationSpecificActivity, Number:RadioactiveActivity, Number:SolidAngle, Number:Speed, Number:Temperature, Number:Time, Number:Volume, Number:VolumePrice, Number:VolumetricFlowRate, Player, Rollershutter, String, Switch]. It must be a value from the enumeration.

..when this was expected:

15: Value Invalid is not facet-valid with respect to enumeration [Call, Color, Contact, DateTime, Dimmer, Group, Image, Location, Number, Number:Acceleration, Number:AmountOfSubstance, Number:Angle, Number:Area, Number:ArealDensity, Number:CatalyticActivity, Number:Currency, Number:DataAmount, Number:DataTransferRate, Number:Density, Number:Dimensionless, Number:ElectricCapacitance, Number:ElectricCharge, Number:ElectricConductance, Number:ElectricConductivity, Number:ElectricCurrent, Number:ElectricInductance, Number:ElectricPotential, Number:ElectricResistance, Number:EmissionIntensity, Number:Energy, Number:EnergyPrice, Number:Force, Number:Frequency, Number:Illuminance, Number:Intensity, Number:Length, Number:LuminousFlux, Number:LuminousIntensity, Number:MagneticFlux, Number:MagneticFluxDensity, Number:Mass, Number:Power, Number:Pressure, Number:RadiationDoseAbsorbed, Number:RadiationDoseEffective, Number:RadiationSpecificActivity, Number:RadioactiveActivity, Number:SolidAngle, Number:Speed, Number:Temperature, Number:Time, Number:Volume, Number:VolumetricFlowRate, Player, Rollershutter, String, Switch]. It must be a value from the enumeration.

[Nadahar]

I think I found the reason for the test failure: openhab/openhab-website#545

The test is hardcoded against the XML schema, which means that the test must be updated when the schema is.

That said, I think it's a bad practice to modify versioned schemas, it kind of undermines everything - the schema version should be bumped instead, not that it would have helped this particular situation.

I've added a commit that updates the test to match the modified schema.

[holgerfriedrich]

Hi, can we go with PMD 7.18.0 or is there a reason to stick to 7.17.0?

[Nadahar]

Hi, can we go with PMD 7.18.0 or is there a reason to stick to 7.17.0?

Yes - I just went to 7.17.0 because that was where the RelianceOnDefaultCharset standard rule I needed was introduced.

[holgerfriedrich]

thanks!

[Nadahar]

Disclaimer: I obviously haven't tested this with 7.18.0, but I assume that there's no reason to expect it to break.

[holgerfriedrich]

@kaikreuzer maybe you could take a look?

[Copilot]

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 9 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Nadahar]

I've renamed the rules as suggested by Copilot, and I've added tests for the rules. I hate to say it, but it was a good thing, because I discovered that I had gotten the "match pattern syntax" wrong for constructors, so they wouldn't have matched. I've taken care of that as well.

[Copilot]

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[holgerfriedrich]

We will need to deal with the deprecations - maybe in a follow-up PR:

[WARNING] Warning at C:\Users\holgerf\src\openhab-core\bundles\org.openhab.core.auth.oauth2client\target\pmd\rulesets\001-rules.xml:35:2
 33|            <priority>3</priority>
 34|    </rule>
 35|    <rule ref="category/java/errorprone.xml/AvoidCatchingNPE">
      ^^^^^ Discontinue using Rule name category/java/errorprone.xml/AvoidCatchingNPE as it is scheduled for removal from PMD. PMD 8.0.0 will remove support for this Rule.

 36|            <priority>2</priority>
 37|    </rule>
[WARNING] Warning at C:\Users\holgerf\src\openhab-core\bundles\org.openhab.core.auth.oauth2client\target\pmd\rulesets\001-rules.xml:38:2
 36|            <priority>2</priority>
 37|    </rule>
 38|    <rule ref="category/java/errorprone.xml/AvoidCatchingThrowable">
      ^^^^^ Discontinue using Rule name category/java/errorprone.xml/AvoidCatchingThrowable as it is scheduled for removal from PMD. PMD 8.0.0 will remove support for this Rule.

 39|            <priority>2</priority>
 40|    </rule>
[WARNING] Warning at C:\Users\holgerf\src\openhab-core\bundles\org.openhab.core.auth.oauth2client\target\pmd\rulesets\001-rules.xml:35:2
 33|            <priority>3</priority>
 34|    </rule>
 35|    <rule ref="category/java/errorprone.xml/AvoidCatchingNPE">
      ^^^^^ Discontinue using Rule name category/java/errorprone.xml/AvoidCatchingNPE as it is scheduled for removal from PMD. PMD 8.0.0 will remove support for this Rule.

 36|            <priority>2</priority>
 37|    </rule>
[WARNING] Warning at C:\Users\holgerf\src\openhab-core\bundles\org.openhab.core.auth.oauth2client\target\pmd\rulesets\001-rules.xml:38:2
 36|            <priority>2</priority>
 37|    </rule>
 38|    <rule ref="category/java/errorprone.xml/AvoidCatchingThrowable">
      ^^^^^ Discontinue using Rule name category/java/errorprone.xml/AvoidCatchingThrowable as it is scheduled for removal from PMD. PMD 8.0.0 will remove support for this Rule.

 39|            <priority>2</priority>
 40|    </rule>

May this will be a bigger revisit of the ruleset anyway. There might be interesting new rules introduced since the ruleset was implemented.
Maybe this goes beyond the deprecation an should be considered for a 1.x release (which also abandons Java11 compat).

[Nadahar]

May this will be a bigger revisit of the ruleset anyway. There might be interesting new rules introduced since the ruleset was implemented.
Maybe this goes beyond the deprecation an should be considered for a 1.x release (which also abandons Java11 compat).

Yeah, I would appreciate it if I didn't have to handle it here, because you can't (shouldn't?) just remove the deprecated rules. I assume that most of them have some kind of replacement, so it's probably a somewhat larger task.

[Nadahar]

As I thought, AvoidCatchingNPE has for example been replaced by AvoidCatchingGenericException - which I must say looks a bit dodgy in the first place. There are legitimate cases for catching NPEs like when using libraries that happen to throw these for some reason that you can't accurately control. RuntimeExceptions are relevant to catch in many circumstances, some people seem to prefer them over checked exceptions to avoid the "forced try", which means that they must often be handled just like a checked exception. Likewise for Exception and Throwable, you are likely to want to catch these to avoid crashing a thread, for example.

So, while there clearly are situations where you can "abuse" all of the above, I'm not sure that they are that suitable to be handled automatically, unless you want code with a lot of suppressions.

[holgerfriedrich]

@Nadahar maybe just replace the two rules in rules.xml with the following snippet (did not test the overall result on the report, but at least the warnings go away):

	<!-- <rule ref="category/java/errorprone.xml/AvoidCatchingNPE"> -->
	<!-- <rule ref="category/java/errorprone.xml/AvoidCatchingThrowable"> -->
	<rule ref="category/java/errorprone.xml/AvoidCatchingGenericException">
		<priority>2</priority>
		<properties>
			<property name="typesThatShouldNotBeCaught" value="java.lang.NullPointerException,java.lang.Throwable"/>
			<!-- match old behavior, ignore ,java.lang.Exception,java.lang.RuntimeException,java.lang.Error -->
		</properties>
	</rule>

[Nadahar]

maybe just replace the two rules in rules.xml with the following snippet

Done.

[Nadahar]

@holgerfriedrich This probably means that all related suppressions in the code must be updated as well.

[holgerfriedrich]

Yes, we will probably find a few ones.... We can proceed once we get a new SAT release published.

[wborn]

In openhab-core there are quite a few findings of these priority 1 rules:

• SetDefaultLocale: 50
• SetDefaultTimeZone: 3

If they aren't fixed the build will fail, do you think they are easy to fix?

[Nadahar]

I'll have to check why they are there. I can't quite understand why these would be set at all. Alternatively, we'll have to lower it to a warning

[Nadahar]

When I search Core, I only find 6 tests that set Locale and 2 that set TimeZone. How did you get the above numbers? Did you try running these rules on all of Core?

[holgerfriedrich]

[ERROR] Failed to execute goal org.openhab.tools.sat:sat-plugin:0.18.0-SNAPSHOT:report (sat-all) on project org.openhab.core:
[ERROR] Code Analysis Tool has found 51 error(s)!
[ERROR] Please fix the error(s) and rerun the build.

I just lowered the level from 1 to 2 for the new rules in sat-plugin/src/main/resources/rulesets/pmd/customrules.xml.
Build is running.... (with you patch plus my SpotBugs PR).

[Nadahar]

Is there a way I can run just the SAT plugin without compiling, running tests etc. on all Core?

[Nadahar]

[ERROR] Failed to execute goal org.openhab.tools.sat:sat-plugin:0.18.0-SNAPSHOT:report (sat-all) on project org.openhab.core:
[ERROR] Code Analysis Tool has found 51 error(s)!
[ERROR] Please fix the error(s) and rerun the build.

I just lowered the level from 1 to 2 for the new rules in sat-plugin/src/main/resources/rulesets/pmd/customrules.xml. Build is running.... (with you patch plus my SpotBugs PR).

I'd still like to see where set locale and timezone fails, to make sure that there's no problem with the detection. Even though I just found this in a few tests, the operation might be done many times in some of the tests, which would explain the difference.

[holgerfriedrich]

With compiling, but a bit faster than just installing:
./mvnw install -Dspotless.check.skip -T3 -DskipTests

[holgerfriedrich]

Quite a lot warnings for [RelianceOnDefaultCharset](https://docs.pmd-code.org/pmd-doc-7.18.0/pmd_rules_java_bestpractices.html#relianceondefaultcharset).
Otherwise nothing unexpected. SimplifyBooleanReturns was removed, and AvoidCatchingGenericException does now no longer match the suppressions in code.

I checked a few of the new RelianceOnDefaultCharset warnings and mostly saw bytebuffer / String conversions without given charset.

[holgerfriedrich]

Suggested change

	<priority>1</priority>
	</rule>
	<rule class="org.openhab.tools.analysis.pmd.SetDefaultTimeZoneRule" name="SetDefaultTimeZone" language="java">
	<priority>1</priority>
	<priority>2</priority>
	</rule>
	<rule class="org.openhab.tools.analysis.pmd.SetDefaultTimeZoneRule" name="SetDefaultTimeZone" language="java">
	<priority>2</priority>

[Nadahar]

Hold on a bit, I'm trying to get an overview over how much work it is to change these tests. Setting the default locale or timezone really is a very bad thing to do, so it would be nice if these could stay at "error".

[Nadahar]

Quite a lot warnings for [RelianceOnDefaultCharset](https://docs.pmd-code.org/pmd-doc-7.18.0/pmd_rules_java_bestpractices.html#relianceondefaultcharset).

edit: Sorry, a bit too quick - I "read" it as reliance on default Locale. I can't quite understand if there aren't many of those. So, the comment below is about that. Regarding Charset, the consequences are usually less after Java 18? when they changed it to be UTF-8 by default, but this can still be changed, so it shouldn't be taken for granted. I'm betting that most of these cases really want UTF-8, so the "fix" is just to specify UTF-8.

That is as expected. It will probably take a long time to get completely rid of these, but in most cases, they are potential bugs, so one have to start some place. In most situations, people configure OH to use the same Locale as their OS/default Locale, but if this isn't the case, many of these may bite. That said, there often won't be a problem even if the Locales are different because many languages share rules, but that's not always the case. Most famous is Turkish where i and I are different letters. There is also quite a lot of difference in number and date formatting.

[Nadahar]

org.openhab.core.internal.items.ItemStateConverterImplTest sets the default Locale multiple places, but from what I can understand, it actually tries to test that the results are not affected when using different locales. It's a bit strange to do it this way, if the code doesn't rely on the default locale, we shouldn't have to "verify" that it doesn't.

I see two problems with the current implementation (we could otherwise just suppress PMD here):

• The original default locale isn't set when the test is over, which means that the test will affect everything that happens after the test.
• If tests are run in parallel, it's hard to know the exact consequences of doing this, since the default locale is JVM-wide.

Any ideas of what the best approach is? It comes from openhab/openhab-core#2362

[holgerfriedrich]

Run on add-ons (1.7 vs 1.8 all PRs):

[Nadahar]

Run on add-ons (1.7 vs 1.8 all PRs):

I had run it on add-ons overnight too, and have made the following commit which takes care of the add-ons: Nadahar/openhab-addons@2454b75

When it comes to Core, I'm still not sure exactly what to do.

The problem is that most of the places where this is used is in the Core types, and they seem to be intricately relying on default Locale. The is also reliance on the default time zone in DateTimeType. To change anything relating to the types, wouldn't just mean to change the tests, but to change the types themselves, because the reason the tests must set the defaults is the fact that the types themselves use the defaults.

I think this is a problem, but I see it as very difficult, if at all possible, to do anything about, and it would require a quite thorough planning and analysis, and should probably be done in a major version change. I assume that changing how these work will break all kind of user scripts and rules. Without changing the types themselves, it's hard to do much with the tests.

The DateTimeType even lacks a format() method that allows specifying both time zone and locale, you're actually forced to rely on the default for one of them, so every current use of it will depend on either the default time zone or the default locale.

The tests for the Core types, unlike the NTP add-on tests, don't even restore the defaults to what they were before the tests, which is especially "bad". I'm thus thinking that maybe the best "fix" at this time would be to suppress them, and add functionality to restore them after tests are done. There is still the challenge of parallel tests though. I have some vague memory that there's some way to tell Maven that a particular test shouldn't run in parallel, and of that is correct, it might be a solution that would make this "safe", but I haven't yet verified if this vague memory is correct or how to do it.

Any ideas about my ideas?

[Nadahar]

I've created openhab/openhab-core#5155 and openhab/openhab-addons#19732 which should allow the setting of default Locale and TimeZone to remain at priority 1.

Actually "resolving" this turned out to be harder than first anticipated in some of the cases, because the code actually relies on the default setting. For other cases, I think it could be removed without much consequence, but as I don't feel that I have the full overview of the cases, I opted to suppress them instead.

@holgerfriedrich @wborn What do you guys think of this approach? Getting rid of reliance on the defaults is a huge task, that can't be done quickly in any case, but having the warnings is at least a start, where this can get some attention. Even if we suppress the setting of the defaults, there will still be warnings where the defaults are (implicitly) used, so the suppression doesn't make the problem "invisible".

[Nadahar]

Regarding parallel tests, there are, as far as I can tell, two challenges:

• JUnit itself can run tests in parallel
• Surefire can launch several tests in parallel, unknown to JUnit.

What I have done in the above PR's only addressed the first case. Surefire runs sequentially be default, but as we know, not if we specify the -T parameter, or potentially if Maven 4 is used, which I think is "parallel by default". As far as I understand, Surefire respects @NotThreadSafe, but since openhab/openhab-core#5033 hasn't gotten any traction, we don't have that annotation available. As I understand it, if @NotThreadSafe is applied to a test class, Surefire will make sure not to run that one in parallel with others.

[Nadahar]

This is after building Core against openhab/openhab-core#5155

[wborn]

Thank you for adding the new checks and resolving the issues they find in the repos!