-
Notifications
You must be signed in to change notification settings - Fork 15
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
base: master
Are you sure you want to change the base?
Conversation
@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. |
This code in the core library is what generates Session IDs and can be followed to the caching of the validation engine: |
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). |
Thanks @dotasek for reviewing. I'll add tests. I'll keep the sessionId then. |
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. |
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