LocalizedMessage should accept message argument in string only

[romani]

Detected by IDEA inspection and Sonar Inspection Make "args" transient or serializable.
https://sonarcloud.io/project/issues?id=org.checkstyle%3Acheckstyle&issues=AW9t2w2MYD2QG1pPXIUJ&open=AW9t2w2MYD2QG1pPXIUJ

Code:

checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/api/LocalizedMessage.java

Lines 84 to 89 in 2f9f8c6

	/**
	* Arguments for MessageFormat.
	*
	* @noinspection NonSerializableFieldInSerializableClass
	*/
	private final Object[] args;

We did not change it as it is API class, so it should be done separately and clear consideration of breaking affects.

Expected: change Object[] to String[] to simply do string concatenation without any thoughts on special format/serialization of particular type.
If some localization is required, it should be done by module/Check that report such violation.

[rnveach]

If some localization is required, it should be done by module/Check that report such violation.

It seems wrong that a class that deals with message localization will not localize the arguments too.

[romani]

I am not sure how it can localize something that he do not understand and can not differentiate, it is just list/array of some string values.
This class is controlling only template string value, so it make good decision on what to use as base, but all other is just some string values that might be not a human words at all.

Do you see example of how localization of arguments can be done and become useful ?

[rnveach]

NonSerializableFieldInSerializableClass

This has now been moved to Violation class.

change Object[] to String[]

This issue (if we are changing the constructor arguments as well) has a large impact on everything from LocalizedMessage and Violation all the way to modules. Modules cannot have integer/enum arguments anymore in their message, they need to be converted to Strings.

Even if we do not change the constructor and only do a conversion in Violation, number formatting in property files are invalid and will result in an exception because it is now a String.

Caused by: java.lang.IllegalArgumentException: Cannot format given Object as a Number
	at java.base/java.text.DecimalFormat.format(DecimalFormat.java:518)
	at java.base/java.text.Format.format(Format.java:158)
	at java.base/java.text.MessageFormat.subformat(MessageFormat.java:1344)
	at java.base/java.text.MessageFormat.format(MessageFormat.java:885)
	at java.base/java.text.Format.format(Format.java:158)
	at com.puppycrawl.tools.checkstyle.LocalizedMessage.getMessage(LocalizedMessage.java:112)
	at com.puppycrawl.tools.checkstyle.api.Violation.getViolation(Violation.java:432)

[rnveach]

@romani ping

Please clarify the extent of the issue. Are we converting the field only, or are we converting the constructor arguments as well?

[romani]

Both. To make it clear that all consideration of how to serialize of arguments should be done outside of this API class. It will print just strings.

[rnveach]

Then this a pretty big break. Method signature is changing, so I am sure all checks will need to be recompiled from source. Sevntu is surely impacted. I wasn't seeing any usages in 3rd parties of Violation but now this extends much further.

[romani]

Yes, I can imagine, we can start with extension of API with new ctor that takes array of string and store it as array of string. And we mark old ctor and field as deprecated to remove in few years.

And we will see migration nuances of sevntu.

[rnveach]

@romani I assume this is all coming solely because this class is serializable and everything associated to it needs to be the same. Can you update issue with why this class needs to remain serializable?

If we are to break things, maybe it makes more sense to just not make this serializable. While yes we don't want to be responsible for how arguments are displayed, the property file can also be configured to handle this as seen from the exception I provided for things like numbers. Truthfully all we are doing for this objects is a to string, which is pretty much common Java imo.

This class was made serializable in d2af4ac , 15 years ago.

[rnveach]

If we continue, then the following will also have to be changed, impacting all our tests.

checkstyle/src/test/java/com/puppycrawl/tools/checkstyle/AbstractModuleTestSupport.java

Line 505 in dcadc13

protected final String getCheckMessage(String messageKey, Object... arguments) {

If all checks are to format their own output now, then they require #12104 as there is no other way to access the current set Locale, as there is no getter. Things like String.format take a Locale. The PRs for it are stuck in code review, so this issue is becoming tied to it.

And we will see migration nuances of sevntu.

Migration nuances can be seen at https://github.com/rnveach/checkstyle/commits/issue_7488_large in our own repo. Sevntu will be duplicating the same.

Currently since we have to format integers ourselves, if we don't do something we lose commas as seen in rnveach@12c4f91 . We now have to call a lot more String.valueOf and toString for integers/enums/classes.

we can start with extension of API with new ctor that takes array of string and store it as array of string

Yes, we can, but output will be damaged (we still must call toString for the Object form) and message properties will be broken that use {0,number,integer} and cause the exception reported before. The only thing this saves us is recompiling the class, but it won't be the same functionally as before or will cause exceptions.

[romani]

and message properties will be broken that use {0,number,integer}

uhh, I missed this. it looks like we can not make this update as this will damage whole world.
I think we need to abandon this issue, it is too aggressive, without much of benefit.

We can extend doc to explain usage of Object, to avoid attempt to do same next time.

[rnveach]

I think we need to abandon this issue, it is too aggressive, without much of benefit.

I agree.

We can extend doc to explain usage of Object, to avoid attempt to do same next time.

Lets keep issue open until this is done, as a reminder.

[romani]

Arguments for java.text.MessageFormat.
Note: Changing types from Object will be
huge breaking compatibility, as Module 
messages use some type formatting 
already, so better to keep it in such way.

@rnveach , is this extension of javadoc is good?

[rnveach]

I am think extension is good.