Refactor workaround for Okapi in-memory document handling#1052
Draft
wadimw wants to merge 2 commits intoupstream-patchedfrom
Draft
Refactor workaround for Okapi in-memory document handling#1052wadimw wants to merge 2 commits intoupstream-patchedfrom
wadimw wants to merge 2 commits intoupstream-patchedfrom
Conversation
Forced URI injection into RawDocument through reflection breaks its contract (stating that RawDocument can backed by one of URI, CharSequence or Stream at a time) and impacts the way it handles content at runtime. Since this workaround is currently only needed for Okapi QualityCheckStep, this commit minimizes its impact. Required property (StartDocument#name) is injected within the existing Mojito's QualityCheckStep subclass, so that only the StartDocument Event is affected. Additionally, this approach does not require Reflection, because the StartDocument resource provides a public method to set its name.
Author
|
TODO should probably also get rid of in this PR |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Split off from #1049
This PR removes the URI injection using reflection on RawDocument which is potentially fragile. Instead, it replaces it with an override on
QualityCheckStepSTART_DOCUMENTevent handler that injects the expected value directly into theStartDocumentresource through its public methodStartDocument#setName.Additionally, it slightly refactors the empty
RawDocumentapproach to make it more explicit that the content of this document is not relevant - it's only used to "drive" the Okapi pipeline and provide Locale configuration.Rationale for the URI refactor:
Force setting URI on a
RawDocumentbacked by InputStream or a CharSequence breaks its contract as specified here:Even the private runtime of RawDocument itself depends on whether the URI field is set (e.g. here), meaning this workaround might introduce unexpected behaviour and is potentially fragile.
After removal of the URI injection from Mojito code, the only thing that breaks seems to be
QualityCheckStep- specifically, it throws a NullPointerException on the following line:This can be avoided by setting an override on our existing
QualityCheckStepsubclass which will set this name if it's null. This way, the workaround is localized only to the class that actually needs it, rather than affecting all steps of Okapi pipelines that would rely onRawDocument.