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.

Sign up for GitHub

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

Jump to bottom

Issue #232 (HL7 2.5.1: Failure to parse ORDER group repetitions in OML_O33) #240

Merged
merged 14 commits into from
Nov 20, 2021
Merged

Issue #232 (HL7 2.5.1: Failure to parse ORDER group repetitions in OML_O33) #240

merged 14 commits into from
Nov 20, 2021

Conversation

ghost
Copy link

@ghost ghost commented Nov 10, 2021

  • Added unit test that exposes the failure to parse ORDER group repetitions.
  • Added a ParserConfiguration class which holds the "non-greedy mode" setting.
  • Updated MessageIterator to handle non-greedy parsing when using PipeParser.
  • Updated PipeParser to hold ParserConfiguration.

@iangralinski-invetech
Issue nHapiNET#232 (HL7 2.5.1: Failure to parse ORDER group repetitio…
435fc85 
…ns in OML_O33)

* Added unit test that exposes the failure to parse ORDER group repetitions.
* Added a ParserConfiguration class which holds the "non-greedy mode" setting.
* Updated MessageIterator to handle non-greedy parsing when using PipeParser.
* Updated PipeParser to hold ParserConfiguration.
Fix static analysis issues (removed constructors with optional parame…
aff4296 
…ters).
milkshakeuk
milkshakeuk reviewed Nov 10, 2021
View reviewed changes
Copy link
Member

@milkshakeuk milkshakeuk left a comment

Choose a reason for hiding this comment

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

I have left a few comments for you to review.

src/NHapi.Base/Parser/MessageIterator.cs Outdated Show resolved Hide resolved
src/NHapi.Base/Parser/MessageIterator.cs Show resolved Hide resolved
src/NHapi.Base/Parser/MessageIterator.cs Outdated Show resolved Hide resolved
src/NHapi.Base/Parser/MessageIterator.cs Outdated Show resolved Hide resolved
src/NHapi.Base/Parser/ParserConfiguration.cs Outdated Show resolved Hide resolved
src/NHapi.Base/Parser/PipeParser.cs Outdated Show resolved Hide resolved
src/NHapi.Base/Parser/PipeParser.cs Outdated Show resolved Hide resolved
IanGralinski-Invetech added 3 commits November 11, 2021 12:11
* Changed Parsers so that ParserConfiguration is passed in when calli…
b07ee0b 
…ng Parse instead of being provided as a constructor parameter.

* Updated MessageIterator to take ParserConfiguration instead of ParserBase in its constructor.
* Other minor updates from review feedback (documentation, naming, etc.)

Issue nHapiNET#232
Merge remote-tracking branch 'origin/master' into issue232_failure_to…
710bbe6 
…_parse_ORDER_group_repetitions_in_OML_O33
Changed default arguments to use overloading to address static analys…
e99123f 
…is errors.

Issue nHapiNET#232
milkshakeuk
milkshakeuk reviewed Nov 11, 2021
View reviewed changes
Copy link
Member

@milkshakeuk milkshakeuk left a comment

Choose a reason for hiding this comment

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

Have responded to your questions and left a few comments, resolved the previous comments where required.

src/NHapi.Base/Parser/ParserBase.cs Outdated Show resolved Hide resolved
tests/NHapi.NUnit/Parser/PipeParserV251Tests.cs Outdated Show resolved Hide resolved
Added default ParserConfiguration as a static member of ParserBase.
a85d5a5 
Replaced call sites where null was being passed in for ParserConfiguration with the default constructed one.

Issue nHapiNET#232
@ghost
Copy link
Author

ghost commented Nov 11, 2021

@milkshakeuk Thanks for taking the time to review my pull request :)

@milkshakeuk
Copy link
Member

@milkshakeuk Thanks for taking the time to review my pull request :)

@iangralinski-invetech no problem!

milkshakeuk
milkshakeuk reviewed Nov 11, 2021
View reviewed changes
Copy link
Member

@milkshakeuk milkshakeuk left a comment

Choose a reason for hiding this comment

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

Thank you for the changes, one more comment added.

milkshakeuk
milkshakeuk reviewed Nov 11, 2021
View reviewed changes
Copy link
Member

@milkshakeuk milkshakeuk left a comment

Choose a reason for hiding this comment

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

Some missing ArgumentNullException xmldoc due to Chained Parse method calls.

src/NHapi.Base/Parser/ParserBase.cs Show resolved Hide resolved
Added unit test to check that exception is thrown when provided Parse…
aee3cfe 
…rConfiguration is null.

Updated XML method documentation for remaining methods that can throw when the provided ParserConfiguration is null.

Issue nHapiNET#232
@milkshakeuk
Copy link
Member

@iangralinski-invetech is there any way we can port these 2 greedy tests from hapi just for the extra coverage.

https://github.com/hapifhir/hapi-hl7v2/blob/3333e3aeae60afb7493f6570456e6280c0e16c0b/hapi-test/src/test/java/ca/uhn/hl7v2/parser/NewPipeParserTest.java#L158

https://github.com/hapifhir/hapi-hl7v2/blob/3333e3aeae60afb7493f6570456e6280c0e16c0b/hapi-test/src/test/java/ca/uhn/hl7v2/parser/NewPipeParserTest.java#L217

If not no pressure I can give it ago.

@github-actions

This comment has been minimized.

Sign in to view
@ghost
Copy link
Author

ghost commented Nov 15, 2021

@iangralinski-invetech is there any way we can port these 2 greedy tests from hapi just for the extra coverage.

https://github.com/hapifhir/hapi-hl7v2/blob/3333e3aeae60afb7493f6570456e6280c0e16c0b/hapi-test/src/test/java/ca/uhn/hl7v2/parser/NewPipeParserTest.java#L158

https://github.com/hapifhir/hapi-hl7v2/blob/3333e3aeae60afb7493f6570456e6280c0e16c0b/hapi-test/src/test/java/ca/uhn/hl7v2/parser/NewPipeParserTest.java#L217

If not no pressure I can give it ago.

I can give it a go, though it might still be a day or two until I can jump back on this.

@milkshakeuk
Copy link
Member

milkshakeuk commented Nov 15, 2021 •
edited
Loading

@iangralinski-invetech I was thinking (as you do before you fall to sleep) and I think we should change the name of ParserConfiguration to Hl7ParserOptions or maybe event just ParserOptions ... as this seems to follow the modern dotnet style, plus if we ever build any convenient service registration for use with IServiceCollection it would fit with that pattern of having an options delegate for things like globally settings the parser options.

e.g.

services.AddControllers()
    .AddJsonOptions(options => options.JsonSerializerOptions.PropertyNamingPolicy = new SnakeCaseNamingPolicy());

or

builder.Services.Configure<JsonOptions>(opt =>
{
    opt.SerializerOptions.PropertyNamingPolicy = new SnakeCaseNamingPolicy());
});

What do you think? Happy to discuss.

@ghost
Copy link
Author

ghost commented Nov 16, 2021

@milkshakeuk I have made an attempt at porting the 2 greedy tests from hapi. The first was relatively straightforward to translate to C#, while the second I'm not 100% sure that I've translated the intent of the test. Does it look correct to you, or have I missed something?

I have also renamed ParserConfiguration to ParserOptions as you suggested :)

milkshakeuk
milkshakeuk reviewed Nov 16, 2021
View reviewed changes
Copy link
Member

@milkshakeuk milkshakeuk left a comment

Choose a reason for hiding this comment

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

in this commit its just variable and parameter names, should probably be parserOptions.

src/NHapi.Base/Parser/DefaultXMLParser.cs Outdated Show resolved Hide resolved
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@milkshakeuk
Copy link
Member

@milkshakeuk I have made an attempt at porting the 2 greedy tests from hapi. The first was relatively straightforward to translate to C#, while the second I'm not 100% sure that I've translated the intent of the test. Does it look correct to you, or have I missed something?

@iangralinski-invetech I don't think it loses its intent, I suspect the empty if blocks in the hapi tests might be just there for when they get example messages for other message types greedy is applicable.

@github-actions
Copy link

Unit Test Results

       5 files     105 suites   16s ⏱️
   991 tests    985 ✔️   6 💤 0 ❌
1 905 runs  1 894 ✔️ 11 💤 0 ❌

Results for commit a247379.

This was linked to issues Nov 19, 2021
HL7 2.5.1: Failure to parse ORDER group repetitions in OML_O33 #232
Closed
Incorrect OML_O21 message parsed #65
Closed
@milkshakeuk milkshakeuk added this to In Progress in NHapi Kanban via automation Nov 19, 2021
@milkshakeuk milkshakeuk added this to the v3.1.0 milestone Nov 19, 2021
@milkshakeuk milkshakeuk merged commit 1b414c6 into nHapiNET:master Nov 20, 2021
NHapi Kanban automation moved this from In Progress to Development Done Nov 20, 2021
@milkshakeuk milkshakeuk moved this from Development Done to Released in NHapi Kanban Jan 1, 2022
@milkshakeuk
Copy link
Member

@PhantomGrazzler I have updated the wiki with details on all the ParserOptions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
No open projects
NHapi Kanban
Released
Milestone
v3.1.0
Development

Successfully merging this pull request may close these issues.

HL7 2.5.1: Failure to parse ORDER group repetitions in OML_O33 Incorrect OML_O21 message parsed
2 participants
@milkshakeuk @iangralinski-invetech