JSON Integration Adapter Code Review 3 Changes

This page may be obsolete.
It contains historical information.

General

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

GenericParser.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

JSONWriter.java

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.	Complete
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.	Complete
Remove writeBoolean(Boolean). It will break if the value is null, and is not really needed. Invoke writeBoolean(boolean) instead.	Complete

JSONMarshaller.java

Tasks	Status
Undo the trivial method renaming	Complete

baseTypes.xsd

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

en.strings

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).	Complete
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}".	Complete
*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?	Complete
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).	Complete
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}".	Complete
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}").	Complete
err.integration.json.parse.requiredValueNull=Error parsing "{0}", required part cannot be "null". -> err.integration.json.parse.requiredNull=Required message part "{0}" is null.	Complete
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).	Complete
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}").	Complete
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.	Complete
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}".	Complete
err.meta.integration.json.invalidTypedArray.notCollection=Typed array message {0} child part must be a collection. -> no, not really. Remove.	Complete
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.	Complete

JSONMessagePartMapping.java

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.	Complete

RootJSONMessagePartMapping.java

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
FORMAT_TYPED_ARRAY -> SUBTYPE_ROOT = 2	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

XMLJSONMessageMappingLoader.java

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	Complete
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"?	Compelte
All regexp pattern should be precompiled in final static constants.	Complete

JSONMessageFormatter.java

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.	Complete
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	Complete
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

JSONMessageParser.java

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

JSONMessageTest.java

Tasks	Status
FORMART -> FORMAT	Complete