Skip to content

Conversation

@rkorytkowski
Copy link

@rkorytkowski rkorytkowski commented Mar 7, 2024

I needed to change from openjdk to amazoncorretto for arm support (Mac m2).

I reuse ValidationEngine between requests since it's thread-safe (no need for sessionId).

I added volatile to ValidationService fields to make the code behave correctly in multi-threaded environments.

Added support for Content-Type/Accept application/fhir+json and application/fhir+xml; fhirVersion=x.x.x

@dotasek
Copy link
Collaborator

dotasek commented Mar 22, 2024

@rkorytkowski the ValidationEngine was not designed with thread safety in mind.

The wrapper makes use of sessions to ensure that ValidationEngines do not interact with each other. New sessions, unfortunately, take some time to load, so once they are loaded, subsequent requests with the same session ID can re-use the already built engine. Each session ID will remain cached for at least 1 hour past its last use.

There is a branch I have not gotten around to fully testing: https://github.com/hapifhir/org.hl7.fhir.validator-wrapper/tree/do-20240126-baseengine which makes this process faster by keeping several commonly used configurations which can be quickly copied for new sessions.

I will take a longer look at the actual code content of this PR as well, but that's the first issue I see.

@dotasek
Copy link
Collaborator

dotasek commented Mar 22, 2024

@dotasek
Copy link
Collaborator

dotasek commented Mar 22, 2024

A basic test of this endpoint seems to indicate that it functions as described (it hits the appropriate convert code in ValidationService).

I would welcome tests built against this, either directly in the codebase, or using IntelliJ's HTTP Client to test the endpoint via http (see the branch I linked above).

Some things I noticed:

This isn't using the CliContext object as a field in the body, favouring query params instead. I would prefer if it did, but I can see an argument for simplicity. But there's also the details below to consider:

One place where this deviates from our current way of operation is that ValidationRequest objects were designed to send multiple files in a single request. There's no copying data to disk, either for receiving input or sending output; it's simply streamed from the request directly, and to the response directly. Looking at the core code, I can see that conversion simply does not have an equivalent to validationService.validateSources(ValidationRequest request).

@rkorytkowski
Copy link
Author

Thanks @dotasek for reviewing. I'll add tests. I'll keep the sessionId then.

@rkorytkowski rkorytkowski marked this pull request as ready for review April 29, 2024 11:42
@rkorytkowski
Copy link
Author

I reworked the implementation.

It no longer requires sessionId since the ValidationEngine is thread safe as per https://github.com/hapifhir/org.hl7.fhir.core/blob/f2fe93a1d7dfe66d1c70a7ef4379ed511a81e1c7/org.hl7.fhir.validation/src/main/java/org/hl7/fhir/validation/ValidationEngine.java#L152 and can be reused for all requests.

Added support for Content-Type header "application/fhir+json; fhirVersion=5.0" and Accept header "application/fhir+xml; fhirVersion=4.0" for source and target formats and versions, which can be overridden by request params.

Added tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants