Issue #12681: removes serializable from violation and audit event

[rnveach]

Issue #12681

We have comments for this class that even say we do no serialize it.

[rnveach]

Test failed at https://dev.azure.com/romanivanovjr/romanivanovjr/_build/results?buildId=12967&view=logs&j=eb841e9f-6e1e-5c29-3ad5-74ec2c658069&t=a9b761e6-b8d5-50cf-467a-54967189cab2&l=1264 .

============

Notes from #4739

Serialization was added at d2af4ac#diff-52efb9bb7c26b577e3885a9539eac7d5R42 titled Apply parts of patch #1952556 fixing some FindBugs bugs. Thanks to Travis Schneeberger.

http://git.net/ml/java.audit.checkstyle.devel/2008-04/msg00067.html

Findbugs does produce this error if I remove the serialization:

[INFO] Class com.puppycrawl.tools.checkstyle.api.AuditEvent defines non-transient non-serializable instance field localizedMessage [com.puppycrawl.tools.checkstyle.api.AuditEvent] In AuditEvent.java SE_BAD_FIELD

This is because AuditEvent is serialized through inheritance of EventObject.

============

Now this is become why is AuditEvent serialized and need EventObject.

It was added in 1d21af3 , 21 years ago, as the first cut.

[rnveach]

Only failure is simple RedundantThis.

I like this solution more than swapping to String[]. Either way, MessageFormat will handle properly formatting the message. Even some of our comments say we don't use serialization so why do we keep it.

From #4739

We need to investigate how to make a fix and minimize affect to users.
nobody use it as serializable

This is way for minimal effect.

[romani]

items:

[rnveach]

@romani ping

[romani]

I am confused by reference of this PR in second commit. I think we can use single issue to explain all reasons and all updates.

[rnveach]

Do you want me to combine both commits? 2nd commit was from CI failure on first commit. It is not a class mentioned in main issue, so I made it a second commit.

[romani]

@rnveach , how we can be confident that this is not HUGE breaking compatibility for Modules and for plugins in our organization ? it will drive bump of first or second digit in version.

[rnveach]

how we can be confident that this is not HUGE breaking compatibility for Modules and for plugins in our organization ?

If you can tell me how this can break 3rd parties, then we should add a CI test to confirm. I can only go by what I think I know, but I know I have been wrong before and accidentally broke 3rd parties when it wasn't expected.

https://github.com/sevntu-checkstyle/sevntu.checkstyle/blob/master/sevntu-checks/config/import-control.xml
Sevntu, our largest 3rd party module, does not directly use Violation or Audit Event. I specifically made the import controls non-wildcards so we can identify which core classes it was using.

https://github.com/checkstyle/sonar-checkstyle/blob/master/config/import-control.xml
Sonar, has a custom audit listener which captures Audit Events.
https://github.com/checkstyle/sonar-checkstyle/blob/2f9d1a95aa97f30af243f2162198a8f43767a302/src/main/java/org/sonar/plugins/checkstyle/CheckstyleAuditListener.java#L95
We didn't change any of the method signatures, and the class isn't custom extended.

We did change the location of the getSource from the super to the main as the super went away. I am not sure the effect on the byte code and JRE connection, but this may break compatibility. This may require unknown 3rd parties who call this method to be source recompiled and there is no way around this. I think they will need to be source recompiled if they extend the changed classes in any way, or if they assign an instance of the changed class to a Serializable only type.

However, Sonar does not use this method and so can't be impacted if this did break compatibility. Even if we did break compatibility with Sonar, they always release a new JAR with every CS release and I don't think users are custom embedding new CS jars manually into Sonar or making 3rd parties on our 3rd party Sonar.

https://github.com/sevntu-checkstyle/sevntu.checkstyle/tree/master/sevntu-checkstyle-sonar-plugin
I don't know what this means for Sevntu's sonar as it is a few versions behind, but if main sonar isn't impacted I would think Sevntu is not either.

https://github.com/checkstyle/eclipse-cs/blob/783ad3c3b27c5c3956de788075d2c59366412767/net.sf.eclipsecs.core/src/net/sf/eclipsecs/core/builder/Auditor.java#L309
Eclipse-CS is the same as Sonar in all ways mentioned above. They do not have an import control yet, but I wasn't see anything manually.

I did not see anyone accessing the Violation class directly, so it's change seem nominal.

[romani]

Thanks a lot for detailed reply.

I am ok to merge. we will update only second digit of our version as change should not affect if usage of our internals is not deep.

[nrmancuso]

I also agree with minor version bump only.

Minor item:

[nrmancuso]

@rnveach failure at https://checkstyle.semaphoreci.com/jobs/24484e3d-790b-4885-a873-161e1fb8d6ce is legit

[rnveach]

Fixed

[nrmancuso]

Good to merge as CI passes. xwiki failure is not related to this PR.