Allon Mureinik has posted comments on this change.
Change subject: core: Add GWT RPC serializable check
......................................................................
Patch Set 2: Code-Review-1
(4 comments)
Mixing reflection and AST is a big no-no.
See explanation inline.
....................................................
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
I wonder if it's not easier to just force class to declare "implements
Serializeable" despite the interface redundancy.
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.
This is a very bad idea.
When running checkstyle there is no guarantee that the code you're examining is
compiled or exists in your classpath.
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"
This should be an external config
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";
The code in the relevant packages does not depend on GWT, so it can never
implement IsSerializable.
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
