SONAR: code quality for 13 release

[Rapster]

Description

The codebase might need some attention since last time we focused on this, on this day:

• 131 critical
• 87 major

More details here: https://sonarcloud.io/project/issues?resolved=false&id=org.primefaces%3Aprimefaces

I see a lot of Make "XXX" transient or serializable" kind of errors, and I think it'd be great to know once and for all what to do with these warnings?

The rest should be quite easy to fix. If you have any questions, suggestions, feel free to share 😉

Describe the solution you would like

See how we can split the work (it's redundant work and not so fun 😅 )

Additional context

Also I see we have some singletons for DT/TT features, I'm planning to remove those for many reasons (essentially it's an anti-pattern, and we don't need to define them as singleton)

[melloware]

I will let Thomas answer the "transient" question but I am all for helping split the work and fixing the others!

[tandraschko]

TBH i would fix some cases where we know for sure that it can NEVER be serializable but after that i would propably just ignore (remove the rule) the "make XXX transient or serializable"
why?
We cant make all fields serializable" e.g. UIComponents are not serializable, so we have to make it transient
For other fields like TreeNode and we should not force a user to make everything serializable

because also JSF itself doesnt serializable the event but the user might - and it might even just works for the most cases, when the user itself e.g. serialize NodeCollapseEvent and the user applied a serializable TreeNode
if serialization fails, then he need just to implement serializeable in its own TreeNode

if we make it transient, the user will never have the coiche to serialize it

[Rapster]

Ok as long we have a good reason to do it I'm cool with that, I've disabled this rule then

We have 20 critical now just by removing this rule 😅

[Rapster]

16 critical and 66 Major left. I'm having a look at ImageCropperRenderer. We have a lot of Remove usage of generic wildcard type. not sure yet how to deal with those

[tandraschko]

i would disable this rule until a user comes up with a related problem someday; we never had any problems

[tandraschko]

i also wonder about java:S2160
we would need to overwrite equals/hashcode and just delegate to the parent (e.g. DotEndPoint)
i dont like that TBH, i would also ignore it

i think this are rules for "normal projects", not for OS frameworks which gets reviews from X people

[melloware]

I agree about the Generics warning I think we should just disable it.

[tandraschko]

will also fix some java:S1181 and deactivate the rule
its by design to catch Throwable

[Rapster]

S2160 makes sense (maybe less relevant in DotEndPoint and co.) but in general I would definitely keep it (specially when it comes to POJOs)

S1452: maybe exclude that rule for org.primefaces.component package (trying to solve that squid can result into ugly stuff, I didn't try, but looking at a component like DataTable, quickly can have a snowball effect)

S1181: how is it by design?

[tandraschko]

S2160: we can add it back but how can we exclude this for this specific classes? i dont like to add something like //CHECKSTYLE:OFF
currently i dont see any case where we should really do it

S1181: we have currently 2 cases only and this can throw a LinkageError/NoClassDefFound

[jepsar]

This change will break premium templates. They (or at least Diamond) come with a custom menu component. In the renderer they do:

        WidgetBuilder wb = getWidgetBuilder(context);
        wb.init("Diamond", menu.resolveWidgetVar(), clientId)
                .attr("statefulScroll", menu.isStatefulScroll());
        wb.finish();

Which no longer works, because init is now private:

https://github.com/primefaces/primefaces/blame/cdd2511c9c786f2fedcec4fa784e625d8f8718e3/primefaces/src/main/java/org/primefaces/util/WidgetBuilder.java#L85

[melloware]

Yep that could just just be..

WidgetBuilder wb = getWidgetBuilder(context);
wb.init("Diamond", menu).attr("statefulScroll", menu.isStatefulScroll());
wb.finish();

[jepsar]

I was about to post that 👍🏻. I'll update the forum thread.

[Rapster]

That's another reason why PrimeTek should put their templates into private repositories, that would help everyone...

[tandraschko]

did we remove it during 13?
if yes, we can also deprecate first to make migration easier