Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue #2424 - add support for non-validating builders / parsers #2447

Merged
merged 8 commits into from
Jun 1, 2021

Conversation

JohnTimm
Copy link
Collaborator

@JohnTimm JohnTimm commented Jun 1, 2021

Changes made:

  • Updated the AbstractBuilder class to include a validating field, a setValidating method, and an isValidating method
  • Updated the FHIRAbstractParser class to include a validating field, a setValidating method, and an isValidating method
  • Updated the FHIRJsonParser.isPropertySupported method to return super.isPropertySupported(name)
  • Updated the FHIRParser interface to expose setValidating and isValidating methods
  • Moved all calls to the ValidationSupport utility class outside of the constructor into a new generated validate method on the respective nested builder class.
    NOTE: This decision was made after taking another look at Effective Java, the Google Protobuf Java library, the javapoet Java library, and this Stack Overflow:
    https://stackoverflow.com/questions/38173274/builder-pattern-validation-effective-java
    Specifically the mention of this quote from Effective Java:

It is critical that they be checked after copying the parameters from the builder to the object, and that they be checked on the object fields rather than the builder fields (Item 39). If any invariants are violated, the build method should throw an IllegalStateException (Item 60).

Example of the new generated validate method:

protected void validate(Account account) {
    super.validate(account);
    ValidationSupport.checkList(account.identifier, "identifier", Identifier.class);
    ValidationSupport.requireNonNull(account.status, "status");
    ValidationSupport.checkList(account.subject, "subject", Reference.class);
    ValidationSupport.checkList(account.coverage, "coverage", Coverage.class);
    ValidationSupport.checkList(account.guarantor, "guarantor", Guarantor.class);
    ValidationSupport.checkReferenceType(account.subject, "subject", "Patient", "Device", "Practitioner", "PractitionerRole", "Location", "HealthcareService", "Organization");
    ValidationSupport.checkReferenceType(account.owner, "owner", "Organization");
    ValidationSupport.checkReferenceType(account.partOf, "partOf", "Account");
}
  • Added call to new validate method from the build method (guarded by validating field):
public Account build() {
    Account account = new Account(this);
    if (validating) {
        validate(account);
    }
    return account;
}
  • Re-generated Json and XML parser to pass the value of their validating field to the AbstractBuilder.setValidating method immediately after the builder class is instantiated (using the builder static factory method). For example:
private Account parseAccount(...) {
    //...<snip>
    Account.Builder builder = Account.builder();
    builder.setValidating(validating);
    //...<snip>
    return builder.build();
}
  • Added generated from method to generated Code subtypes to be consistent with generated model classes
  • Updated FHIRPersistenceJDBCImpl.convertResourceDTO method to use FHIRParser.setValidating(false) so that resources read from the database do not use a "validating" parser.
  • Updated FHIRParserBenchmark to include benchmarks for both validating and non-validating Json and XML parsers.

Here are the results after running the updated benchmark with 5 random spec examples:

operationdefinition-example
Benchmark                                                     Mode  Cnt      Score      Error  Units
FHIRParserBenchmark.benchmarkJsonParser                      thrpt    4   6999.749 ±  743.366  ops/s
FHIRParserBenchmark.benchmarkJsonParserNonValidating         thrpt    4  19261.375 ± 4047.486  ops/s
FHIRParserBenchmark.benchmarkXMLParser                       thrpt    4   4727.621 ±  701.067  ops/s
FHIRParserBenchmark.benchmarkXMLParserNonValidating          thrpt    4   8174.837 ± 1010.988  ops/s

library-zika-virus-intervention-logic
Benchmark                                                     Mode  Cnt      Score      Error  Units
FHIRParserBenchmark.benchmarkJsonParser                      thrpt    4   8561.627 ±  765.289  ops/s
FHIRParserBenchmark.benchmarkJsonParserNonValidating         thrpt    4  11228.661 ±  914.804  ops/s
FHIRParserBenchmark.benchmarkXMLParser                       thrpt    4   6474.798 ±  823.414  ops/s
FHIRParserBenchmark.benchmarkXMLParserNonValidating          thrpt    4   7531.063 ± 2309.640  ops/s

detectedissue-example-dup
Benchmark                                                     Mode  Cnt      Score       Error  Units
FHIRParserBenchmark.benchmarkJsonParser                      thrpt    4  17946.689 ±  3762.102  ops/s
FHIRParserBenchmark.benchmarkJsonParserNonValidating         thrpt    4  41626.245 ±  4491.693  ops/s
FHIRParserBenchmark.benchmarkXMLParser                       thrpt    4  11651.696 ±   370.050  ops/s
FHIRParserBenchmark.benchmarkXMLParserNonValidating          thrpt    4  19683.286 ± 10744.125  ops/s

careplan-example-f002-lung
Benchmark                                                     Mode  Cnt      Score      Error  Units
FHIRParserBenchmark.benchmarkJsonParser                      thrpt    4   7267.189 ± 4378.650  ops/s
FHIRParserBenchmark.benchmarkJsonParserNonValidating         thrpt    4  18340.629 ± 6584.793  ops/s
FHIRParserBenchmark.benchmarkXMLParser                       thrpt    4   5744.205 ± 2573.052  ops/s
FHIRParserBenchmark.benchmarkXMLParserNonValidating          thrpt    4  10407.999 ± 3061.489  ops/s

encounter-example-emerg
Benchmark                                                     Mode  Cnt      Score      Error  Units
FHIRParserBenchmark.benchmarkJsonParser                      thrpt    4  11601.736 ± 1988.297  ops/s
FHIRParserBenchmark.benchmarkJsonParserNonValidating         thrpt    4  15088.933 ± 2637.177  ops/s
FHIRParserBenchmark.benchmarkXMLParser                       thrpt    4   8221.663 ± 1575.012  ops/s
FHIRParserBenchmark.benchmarkXMLParserNonValidating          thrpt    4  10661.899 ± 1101.927  ops/s

As you can see, there is a very tangible performance boost when using non-validating parsers for both Json and XML.

Signed-off-by: John T.E. Timm johntimm@us.ibm.com

Signed-off-by: John T.E. Timm <johntimm@us.ibm.com>
@JohnTimm JohnTimm requested review from prb112 and lmsurpre June 1, 2021 16:07
Signed-off-by: John T.E. Timm <johntimm@us.ibm.com>
@JohnTimm JohnTimm changed the title Issue #2424 - add support for non-validating builders/parsers Issue #2424 - add support for non-validating builders / parsers Jun 1, 2021
Copy link
Contributor

@prb112 prb112 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - reviewed with John on WebEx

Signed-off-by: John T.E. Timm <johntimm@us.ibm.com>
Signed-off-by: John T.E. Timm <johntimm@us.ibm.com>
Signed-off-by: John T.E. Timm <johntimm@us.ibm.com>
Signed-off-by: John T.E. Timm <johntimm@us.ibm.com>
Signed-off-by: John T.E. Timm <johntimm@us.ibm.com>
Signed-off-by: John T.E. Timm <johntimm@us.ibm.com>
Copy link
Member

@lmsurpre lmsurpre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@JohnTimm JohnTimm merged commit c27ccc8 into main Jun 1, 2021
@JohnTimm JohnTimm deleted the johntimm-main branch June 1, 2021 20:43
tbieste pushed a commit that referenced this pull request Jun 9, 2021
* Issue #2424 - add support for non-validating builders/parsers

Signed-off-by: John T.E. Timm <johntimm@us.ibm.com>

* Issue #2424 - update copyright header

Signed-off-by: John T.E. Timm <johntimm@us.ibm.com>

* Issue #2424 - updates per PR feedback

Signed-off-by: John T.E. Timm <johntimm@us.ibm.com>

* Issue #2424 - updated Javadoc

Signed-off-by: John T.E. Timm <johntimm@us.ibm.com>

* Issue #2424 - introduce ignoringUnrecognizedElements field

Signed-off-by: John T.E. Timm <johntimm@us.ibm.com>

* Issue #2424 - updated unit test

Signed-off-by: John T.E. Timm <johntimm@us.ibm.com>

* Issue #2424 - fixed issue with Builder.validate method

Signed-off-by: John T.E. Timm <johntimm@us.ibm.com>

* Issue #2424 - updated Javadoc wording

Signed-off-by: John T.E. Timm <johntimm@us.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants