JSON Integration Adapter Code Review 3 Changes

From CDOT Wiki
Revision as of 11:14, 4 February 2012 by Gbatumbya (talk | contribs) (JSONMessageFormatter.java)
Jump to: navigation, search


The most important changes are:

  • Changing the terminology and the mapping property type around the mapping modes.
  • Removing the circular ref and OID support, since they do not make sense for the integration engine.
  • Refactoring of the message parser to remove the duplicated parsing logic (there are two possible approaches with this)
  • Spaces generally must not be removed from existing files, as a trivial change and a goal in itself, since it makes it very hard to apply patches to already modified code.

These include src/nexj/core/scripting/GenericParser.java and /core/test/nexj/core/integration/format/xml/XMLMessageParserTest.java

Tasks Status
Please undo (I have found only trivial space-removal changes) Complete

XML_FormatString.message, XMLMessageParserTest.java
Tasks Status
Why have these been changed? Please undo. Complete

Tasks Status
Please undo all the method renaming - this breaks a number of classes in the enterprise project, and was not needed in the first place.

Trivial code modifications are discouraged, since they tend to complicate merges and sometimes, like in this case, even break code.

Remove TOKEN_END_OBJECT and TOKEN_END_ARRAY public constants.

The object and array should be closed by invoking a corresponding method on JSONWriter. Having these constants exposes implementation details and breaks any attempts to subclass this class.

Remove writeBoolean(Boolean). It will break if the value is null, and is not really needed. Invoke writeBoolean(boolean) instead. Complete

Tasks Status
Undo the trivial method renaming Complete

Tasks Status
Array is JSON - leave only one space after JSON Complete
Node -> message part Complete
'It is also called "typed array".' -> remove this phrase Complete
typed array -> root. Put this after the array. Complete
mode -> subtype Complete
root". The subtype should be empty by default. Complete
We should have a "name" attribute for mapping object part keys, as well as subtypes for dates and binary values, like in the XML formatter Complete

Tasks Status
err.integration.json.parse.binary.expectedString=Error parsing binary for "{0}", was expecting a string value but found\: {1}.

-> err.integration.json.parse.binary=Invalid binary value for message part "{0}" (expected a string).

Do not include actual data values in error messages, for privacy reasons.

err.integration.json.parse.binary.invalidBase64=Invalid base64 format for message part "{0}".
-> err.integration.json.parse.base64=Invalid base64 value for message part "{0}".

*err.integration.json.parse.duplicateKey=Found duplicate key\: "{0}".

-> err.integration.json.parse.duplicateKey=Duplicate object key\: "{0}".

  • Can you include the message part path in the message?
err.integration.json.parse.oidExpectedString=Error parsing OID for "{0}", was expecting a string value but found\: {1}.

-> err.integration.json.parse.oid =Invalid OID for message part "{0}" (expected a string).

err.integration.json.parse.oidInvalidBase64=Error parsing OID for "{0}", invalid base64 format at\: Line "{1}", Column "{2}".

-> err.integration.json.parse.base64= Invalid base64 value for message part "{0}".

err.integration.json.parse.primitiveTypeMismatch=Error parsing "{0}", was expecting a {1} value but found\: {2}.

-> err.integration.json.parse.typeMismatch=Type mismatch in message part "{0}" (expected a "{1}").

err.integration.json.parse.requiredValueNull=Error parsing "{0}", required part cannot be "null".

-> err.integration.json.parse.requiredNull=Required message part "{0}" is null.

err.integration.json.parse.timestamp=Error parsing timestamp for "{0}", was expecting a number value but found\: {1}.

-> err.integration.json.parse.timestamp=Invalid timestamp for message part "{0}" (expected a numeric value).

err.integration.json.parse.unexpectedToken=Error parsing "{0}", was expecting "{1}" but found "{2}".

-> err.integration.json.parse.unexpectedToken=Unexpected token in message part "{0}" (expected "{1}").

err.meta.integration.json.anyOrdinalCannotHaveFormat=Primitive message part "{0}" of type any ordinal cannot have a format string.

-> err.meta.integration.json.anyFormat=Primitive message part "{0}" of type any cannot have a format pattern.

err.meta.integration.json.invalidMessageFormatMode=Invalid message format mode "{0}" in RootJSONMessagePartMapping for "{1}".

-> err.meta.integration.json.invalidSubtype=Invalid subtype "{0}" in the mapping of message part "{1}".

err.meta.integration.json.invalidTypedArray.notCollection=Typed array message {0} child part must be a collection.

-> no, not really. Remove.

err.meta.integration.json.invalidTypedArray.notOnePart=Typed array message {0} may only have one child part.

-> err.meta.integration.json.rootChild =Root message part "{0}" must have exactly one child part.


Tasks Status
move m_sFormat, m_bQuote up, into the // attributes section - this is a coding convention Complete
m_bQuote -> m_bQuoted Complete
ToBeQuoted -> Quoted Compelte
Group setter and getters together logically, in the beginning of the operations: setter1 getter1 setter2 getter2...

Having the operations grouped alphabetically makes it hard to read the code.


Tasks Status
RootJSONMessagePartMapping -> CompositeJSONMessagePartMapping (since it is not just for the root message part, right?) Complete
m_nMessageFormat -> m_nSubtype Complete
Defaults to {@link RootJSONMessagePartMapping#FORMAT_OBJECT}. - remove the comment and assign the actual value instead. Complete
static final -> final static (convention) Complete
FORMAT_ -> SUBTYPE_ Complete
Move constants up, before the attributes Complete
MessageFormatMode -> Subtype; The validation is not needed, since it will be validated in the loader when parsing the symbol value. Complete

Tasks Status
throw new MetadataException("err.meta.integration.json.anyOrdinalCannotHaveFormat", new Object[]{part.getFullPath()});
  • Try to do this validation in the mapping instead
  • The same applies to the format validation and the business logic for setting the quoted flag - move to the mapping

if (!"true".equals(sFormat) || !"false".equals(sFormat)) -> this always evaluates to true - please fix the logic.
A format that is just "true" or just "false" seems uncommon though. Did you mean "true;false"?

All regexp pattern should be precompiled in final static constants. Complete
Tasks Status
We should format or parse message part without OIDs and circular refs.

The integration engine is designed to deal with hierarchical messages and circular ones can get it into faulty code paths.
It is supposed to support only the most basic conventions and not output or expect messages that require special handling from the external system.
This is needed to facilitate easy integration. Circular refs and OIDs are used in our RPC marshaller, but the goals of the integration engine are quire different.

m_objectMap - not needed anymore Complete
formatCompositeMessagePart - instead of using nChar, invoke the corresponding writer output methods. Complete
Remove the OID management code Complete
Add code for handling the primitive subtypes (hex, base64, dateTime, date, time) Complete

|| ((nMinCount == nMaxCount) && (nCount != nMinCount)) this condition is invalid and will produce an incorrect error message

Do not use j as a counter. Use k instead. j looks like i under many fonts. Complete
writePrimitive - use a switch for nPartType, rather than several if statements. Complete
Tasks Status
m_objectMap - not needed anymore Complete
The overridden methods, like parseObject(), parseElement(), parseArray() etc have a very similar logic to the superclass methods. To avoid duplication, it is better to handle any custom logic needed in these with template methods, like constructing a TransferObject instead of a hashtab e.g. Object createObject(), addObjectValue(Object obj, String sKey, Object value), createArray(), Object addArrayValue(Object array, Object value).

The overriden methods parse* will save and restore the current message part and will check if the structure they parse corresponds to the current message part type and mapping.
This should not introduce a dependency on the integration classes in JSONParser.
Keep the template methods simple.
Alternatively, do not override the base class parse* methods, but just convert the parsing results to transfer objects on a second pass (this can help with message tables, see below).

Complete (Implemented a hybrid, message table parsing uses a second pass, while direct parsing uses overridden methods)
It should be possible to parse a primitive message, e.g. 1 or "abc". The mapping for this has subtype="root" and the only message part corresponds to this primitive type. Complete
parsePartValue - m_currentMessagePart .isCollection() - remove the space Complete
initializeMessageTable - it is possible to support more than one message with an object subtype. The mapping can support an additional property key="<key>:<value>", designating a key and a value, which must establish a unique combination for identifying the message in the table. This way we can have also message polymorphism (later phase?). Complete

Tasks Status