Vojtech Szocs has posted comments on this change.
Change subject: core: Add GWT RPC serializable check
......................................................................
Patch Set 2:
(4 comments)
Allon, please see my comments inline.
I think there's a way to implement this check as AST-use-only, however it will
significantly improve complexity of this check.
There are already some standard Checkstyle checks using Reflection anyway.
Maybe I should just abandon this patch and split it up into smaller checks,
i.e. "must have default constructor" check, etc.
....................................................
Commit Message
Line 11: for Serializable User-defined Classes [1]:
Line 12:
Line 13: * is assignable to Serializable or IsSerializable interface,
Line 14: either because it directly implements one of these interfaces
Line 15: or because it derives from a superclass that does
>From this check's perspective, yeah, forcing redundant "implements
>SERIALIZABLE_INTERFACE" would be easier.
>From existing Java code perspective, IMHO such thing wouldn't make any sense
>at all. It would introduce change and confusion to existing code.
Instead, since there's only one Check instance used when checking Java classes
(i.e. check is stateful), we could cache info about which class should
implement SERIALIZABLE_INTERFACE and which class really does that, and at the
end of checking process, validate the cache.
Line 16: * all non-final, non-transient instance fields are themselves
Line 17: serializable
Line 18: * has a default (zero argument) constructor (with any access
Line 19: modifier) or no constructor at all
Line 31:
Line 32: GwtRpcSerializableCheck uses Java Reflection API instead of
Line 33: Checkstyle's AST Token API. This is because of advanced class
Line 34: introspection (i.e. check if given class implements interface
Line 35: X either directly or indirectly) required by this check.
Conceptually, I agree with you. Mixing AST and Reflection is bad idea in
general.
However, static code analysis (i.e. AST) is not enough for the kind of checking
necessary here. Even if we eliminate use of Reflection for "implements
SERIALIZABLE_INTERFACE" part (see my comment above), we still have "fields are
serializable" part - this includes inherited fields. Sure, we can do this using
some kind of internal cache as well, however it will significantly complicate
the check implementation. Complex code = bigger potential for bugs.
BTW, if you look at AbstractTypeAwareCheck, it's the base class for resolving
class metadata via Reflection. AbstractTypeAwareCheck has (at least) two
implementations:
* JavadocMethodCheck
* RedundantThrowsCheck
In other words, in some cases, using AST is just too cumbersome, and Checkstyle
provides Reflection support via AbstractTypeAwareCheck. That's the reason why
above mentioned checks use Reflection after all.
Line 36:
Line 37: GwtRpcSerializableCheck makes best effort not to load/check
Line 38: given class more than once.
Line 39:
....................................................
File
build-tools-root/ovirt-checkstyle-extension/src/main/java/checks/GwtRpcSerializableCheck.java
Line 33: private static final String[] RPC_SERIALIZABLE_ROOTS = {
Line 34:
"org.ovirt.engine.core.common.queries.VdcQueryParametersBase",
Line 35:
"org.ovirt.engine.core.common.action.VdcActionParametersBase",
Line 36: "org.ovirt.engine.core.common.users.VdcUser",
Line 37:
"org.ovirt.engine.core.common.businessentities.BusinessEntity"
Yeah, I tried putting extra config properties in checkstyle.xml for this check,
however I faced weird issues after that, i.e. check wasn't invoked at all.
(For some strange reason, besides "run" property, any other property doesn't
work. I guess I'm missing something.)
Line 38: };
Line 39:
Line 40: /**
Line 41: * Types to exclude from checking in addition to
<code>java.lang.*</code> classes:
Line 49: "java.util.UUID"
Line 50: };
Line 51:
Line 52: public static final String JAVA_SERIALIZABLE =
"java.io.Serializable";
Line 53: public static final String GWT_ISSERIALIZABLE =
"com.google.gwt.user.client.rpc.IsSerializable";
This check was meant as general-purpose reusable GWT RPC "Serializable
User-defined Classes" check, which is why it also supports checking
IsSerializable interface.
If IsSerializable.class cannot be resolved (GWT JAR not on classpath during
Checkstyle invocation), it will simply skip check for this interface.
Line 54:
Line 55: private final Map<String, Class<?>> typeCache = new
HashMap<String, Class<?>>();
Line 56: private final Set<String> typesChecked = new HashSet<String>();
Line 57:
--
To view, visit http://gerrit.ovirt.org/18910
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I51c87efc432444226daa7a18be047435d21b84ef
Gerrit-PatchSet: 2
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Vojtech Szocs <[email protected]>
Gerrit-Reviewer: Allon Mureinik <[email protected]>
Gerrit-Reviewer: Einav Cohen <[email protected]>
Gerrit-Reviewer: Tal Nisan <[email protected]>
Gerrit-Reviewer: Vojtech Szocs <[email protected]>
Gerrit-Reviewer: Yair Zaslavsky <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
