View source for JSON Integration Adapter Code Review 3 Changes

[[category: NexJ Express JSON Message Adapter]]

===== 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=====
{| border="1" cellpadding="4" cellspacing="2" style="border: 1px solid black;border-collapse:collapse;"
|-
! Tasks
! Status
|-
| Please undo (I have found only trivial space-removal changes)
|Complete
|}


=====XML_FormatString.message, XMLMessageParserTest.java=====
{| border="1" cellpadding="4" cellspacing="2" style="border: 1px solid black;border-collapse:collapse;"
|-
! Tasks
! Status
|-
|Why have these been changed? Please undo.
|Complete
|}


=====JSONWriter.java=====
{| border="1" cellpadding="4" cellspacing="2" style="border: 1px solid black;border-collapse:collapse;"
|-
! 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=====
{| border="1" cellpadding="4" cellspacing="2" style="border: 1px solid black;border-collapse:collapse;"
|-
! Tasks
! Status
|-
| Undo the trivial method renaming
|Complete
|}


=====baseTypes.xsd=====
{| border="1" cellpadding="4" cellspacing="2" style="border: 1px solid black;border-collapse:collapse;"
|-
! 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
|-
|Use an enum instead of an integer for mode, this is far more developer friendly, and does not need a coding table: e.g. subtype="array|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=====
{| border="1" cellpadding="4" cellspacing="2" style="border: 1px solid black;border-collapse:collapse;"
|-
! Tasks
! Status
|-
| err.integration.json.parse.binary.expectedString=Error parsing binary for "{0}", was expecting a string value but found\: {1}. <br/>
-> 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. <br/>
err.integration.json.parse.binary.invalidBase64=Invalid base64 format for message part "{0}". <br/>
-> err.integration.json.parse.base64=Invalid base64 value for message part "{0}".
|Complete
|-
|*err.integration.json.parse.duplicateKey=Found duplicate key\: "{0}". <br/>
-> 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}.<br/>
-> 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}". <br/>
-> 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}. <br/>
-> 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". <br/>
-> 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}. <br/>
-> 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}". <br/>
-> 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.<br/>
-> 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}". <br/>
-> 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. <br/>
-> no, not really. Remove.
|Complete
|-
|err.meta.integration.json.invalidTypedArray.notOnePart=Typed array message {0} may only have one child part. <br/>
-> err.meta.integration.json.rootChild =Root message part "{0}" must have exactly one child part.
|Complete
|}


=====JSONMessagePartMapping.java=====
{| border="1" cellpadding="4" cellspacing="2" style="border: 1px solid black;border-collapse:collapse;"
|-
! 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... <br/>
Having the operations grouped alphabetically makes it hard to read the code.
|Complete
|}


=====RootJSONMessagePartMapping.java=====
{| border="1" cellpadding="4" cellspacing="2" style="border: 1px solid black;border-collapse:collapse;"
|-
! 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=====
{| border="1" cellpadding="4" cellspacing="2" style="border: 1px solid black;border-collapse:collapse;"
|-
! Tasks
! Status
|-
|<code>throw new MetadataException("err.meta.integration.json.anyOrdinalCannotHaveFormat", new Object[]{part.getFullPath()});</code>
*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
|-
|
<code>if (!"true".equals(sFormat) || !"false".equals(sFormat))</code> 
-> this always evaluates to true - please fix the logic. <br/>
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=====
{| border="1" cellpadding="4" cellspacing="2" style="border: 1px solid black;border-collapse:collapse;"
|-
! Tasks
! Status
|-
|We should format or parse message part without OIDs and circular refs. <br/>
The integration engine is designed to deal with hierarchical messages and circular ones can get it into faulty code paths. <br/>
It is supposed to support only the most basic conventions and not output or expect messages that require special handling from the external system. <br/>
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. <br/>
|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
|-
|
<code>|| ((nMinCount == nMaxCount) && (nCount != nMinCount))</code> 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=====
{| border="1" cellpadding="4" cellspacing="2" style="border: 1px solid black;border-collapse:collapse;"
|-
! 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). <br/>
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. <br/>
This should not introduce a dependency on the integration classes in JSONParser. <br/>
Keep the template methods simple. <br/>
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=====
{| border="1" cellpadding="4" cellspacing="2" style="border: 1px solid black;border-collapse:collapse;"
|-
! Tasks
! Status
|-
| FORMART -> FORMAT
|Complete
|}