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

Implement /convert endpoint for xml/json conversion (#165) #167

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

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

This code in the core library is what generates Session IDs and can be followed to the caching of the validation engine:

https://github.com/hapifhir/org.hl7.fhir.core/blob/5cc34ec77d787f62dd640ee08b5870ff1f817f2e/org.hl7.fhir.validation/src/main/java/org/hl7/fhir/validation/cli/services/ValidationService.java#L95

@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
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