Menu
▾
▴
#163 ValidationContext inconsistencies
Both Parser and AbstractMessage have ValidationContext member variables, but they can be different:
ORU_R01 r01 = new ORU_R01();
String message = .... // some message with invalid primitives
r01.parse(message); // passes, because the validation context of the message is null
r01 = r01.getParser().parse(message) // fails, because the validation context of the parser is DefaultValidation
In other words: the default ValidationContext for Parsers is an instance of DefaultValidation, while the default ValidationContext for (newly created) message instances is null (and the latter context is only used by AbstractPrimitive#setValue) . Is this intended? Is there some intended kind of "override" mechanism?
Personally, if there's no need for message-instance based validation, I would prefer to remove the validation context field of the message and return getParser().getValidationContext() instead.
As a consequence, the following would happen:
ADT_A01 adt = new ADT_A01();
MSH mshSegment = adt.getMSH();
mshSegment.getSequenceNumber().setValue("GABLORG"); // immediately fails, because number pattern (NM) is enforced now
Currently, such a message can be constructed and encoded without error.
FWIW I'd definitely agree with your suggestion to remove validation from individual messages. In the end this situation only exists because Messages didn't previously hold a reference to their a parser, so holding a validation context was the only way they could enforce values. Now that they do, it makes sense to change the place where validation comes from.
Perhaps the right approach would be to deprocate the message-specific methods for 2.1 with a recommendation to use HapiContext for that, and then cut it out entirely for 3.0?
One more idea- Maybe it makes more sense to simply have the message store the Parser instance, but have the parser instance keep a reference to its HapiContext and have the parser provide a reference to HapiContext which in turn provides the ValidationContext any time it's needed (e.g. when ST#setValue() gets called). The benefit I see here is that changes to the validation context of HapiContext would then take effect immediately, as opposed to only working on new parser instances. Although maybe that would be more misleading...
Also, I wonder if it makes sense to do the same for ModelClassFactory?
... and the ParserConfiguration.
I'm not entirely sure yet if a HapiContext should be a configuration blueprint applied to Parsers, Servers etc. (they way it's implemented now) with the degree of freedom to modify configs without affecting the HapiContext, or if it should be the unique configuration reference for all of these objects.
The ValidationContext in message instances is just another degree of config copying, which we should definitely get rid of.
On second thought I think that HapiContext should be the only reference.
As a consequence, Parsers etc. will only reference a HapiContext, not anymore ValidationContext, ModelClassFactory etc. separately.
Constructors like PipeParser(ModelClassFactory) will create a DefaultHapiContext on the fly and modify its ModelClassFactory; but I'll add a note in the javadoc that PipeParser(HapiContext) is preferred.
Implemented - HapiContext is now the central configuration hub - changing any properties there immediately affects all objects that were or will be created from HapiContext.
Fixed a lot of tests and neraly all examples to use HapiContext.
The message-private ValidationContext have been marked as deprecated, however, the parser injects its own ValidationContext into the message as default.
FYI I've checked in a change to HapiContext, comments are welcome.
It occurred to me that we could make Hapi a whole lot friendlier to beginners by just renaming the getSimpleService and getTwoPortService to getServer(port) and getServer(port, port), and then abstracting away the ConnectionHub behind getClient(port) and getClient(port, port) methods.
Then it occurred to me that since unlike the entire rest of HapiContext, these methods are factories instead of accessors, they should be called newClient and newServer.
+1, except it seems that you committed to the old repo?