25 from until fixes#28
Merged
poikilotherm merged 75 commits intobranch-5.0from Jun 10, 2022
Merged
Conversation
Instead of creating multiple different hierarchies for exceptions, now common has a topmost OAIException that all hierarchies will be attached to. This commit moves also the BadArgument, BadVerb and BadResumptionToken into the common package, as these are very basic for every operation. Also, the OAIException is now abstract, so you need to create more specific exceptions when problems arise. The newly introduced abstract method getErrorCode() forces all exceptions to be associated with a certain OAI-PMH error code, and enables better messages.
These are used by all exceptions inheriting from OAIException to compose better, human-friendly error messages. This will obsolete the large if/else construction within xoai-data-provider/ErrorHandler.
All verbs are now moved into their own package to make the model classes more comprehensable. It is more clear now, what these are for. The Verb interface has been extended: 1. To carry an enumerated list of OAI-PMH specified Arguments (copied and refactored from the xoai-data-provider/OAIRequest.Parameters enum) 2. Associate each verb type within Verb.Type enum with sets of required, optional and exclusive arguments, following the OAI-PMH 2.0 spec. This will allow for better validation when parsing queries if all arguments are present and allowed. (It does not yet add validation of the argument value)
For easier exception catching and being more aligned with the underlying StAX writer, make the XmlWriteException a subclass. This will be especially important to create new methods in XmlWriter that reference to other method throwing one of both. A common ancestor helps with that.
Instead of using different instance values for the attributes of <request>, use an EnumMap to store them, reusing the Verb.Argument enum introduced earlier. Instead of returning null values, use the Optional util to make the API more fluent. Reuse this to extend XmlWriter with a writeAttribute() method taking Verb.Arguments and Optional as arguments - unwrapping happens in there, lowering the complexity of the Request.write(). This also avoid repeated strings for the attribute name, as we reuse already checked and defined ones from the Verb.Argument enum.
- Allow parsing to accept both Granularity.Second or Granularity.Day - Formats like Granularity.Second
- When calling get...() throw an InternalOAIException when the setting has not been set before (will cause a servlet error 500 if not catched) - Seal the class: make the values immutable when getting, but editable by the with() methods
The OAI-PMH <request> response may consist of just the base URL when the verb is invalid. The class needs to reflect that. Reusing the already present Optional<> uses for it.
- For a better overview of exceptions, those directly associated to OAI-PMH status codes have been moved into a separate package. - The exception inheritance hierarchy for these now also reflects better their nature: inheriting from an abstract HandlerException that itself inherits from xoai-common/OAIException makes these more streamlined. - The abstract OAIException also requires these exceptions to implement the status code method getErrorCode(), which makes them much easier to handle within the ErrorHandler later on
- Remove the old compilation approach
- Add a new builder, incorporating the changes within the common module:
- Make heavy use of the Verb.Type and Verb.Argument enums to walk
through the parameters
- Prepare for jakarta.servlet.ServletRequest.getParameters()
returning a Map<String, String[]>
- Rely on the Granularity configuration for robust from/until
parsing
- Check for allowed, required, optional and exclusive arguments in a
much more robust and concise way
- Try our best to create lists of errors to allow users to fix their
requests faster (as requested by the spec)
- The new builder relies on static methods and is sealed.
Using applications may provide their own builders and/or reuse parts
of the methods here. The common interface is the final class
RequestBuilder.RawRequest, which will be used within the DataProvider, too.
- The classes are made to be non-mutable by default.
- The ErrorHandler is rewritten to make use of the new OAIException.getMessage() and OAIException.getErrorCode() interface, dramatically reducing the complexity. - It can now interact with a given OAI-PMH response model object and RawRequests to build the error response that gets sent to users
To avoid running into the limitations of Java that a superclass cannot return references to itself as instances of the subclass, remove the OAIRequest class again. Tne resumption token loading will happen inside handlers, reading the value from the string.
- VerbHandler:
- Remove all methods referring the removed OAIRequest/OAICompilesRequest/Builders
- Simplify receiving the config by adding theConfiguration() method
- Do not accept XmlWritables as generic type, but Verbs only!
- DataProvider:
- Provide different entry points into the analysis of requests.
You can start with the raw parameters from the servlet, with a
raw request or with a completely made up request.
- The different entry points require you to take care of exceptions
yourself.
- They make use of the much more convenient ErrorHandler to generate
error responses
- The class has been aligned to the new exception hierarchy.
- The basic testing infrastructure, DataProviderTest and
AbstractHandlerTest have also been aligned
Better reuse our exceptions than use outside ones.
Member
Author
|
(Of course this is not ready - the handlers need to be fixed and aligned to the new structure yet. And we need tests. TESTS 😄 ) |
- Add Javadocs hints to use this class in applications - Refactor to not throw any OAIExceptions, but always return OAIPMH responses (with errors if necessary) - Refactor to avoid users need to provide an OAIPMH instance beforehand. (Can manipulate still after the fact) - Change test accordingly
Due to the reorganization of exceptions, the repository shall only throw errors that are related to item handling. Things like a bad verb etc are dealt with by the lib - the repository sees validated requests only.
The FilterResolver was a wrapper inside a wrapper. The Condition interface is already making sure people will create their own Filter providers. The Repository class had a private FilterResolver field, with a getter only, resulting in always null and not changeable by inheriting classes.
- Remove the hasCondition() and getCondition() from MetadataFormat, Set and Context - Replace with isItemShown(), using the condition if present (each of the model classes have only one condition), rendering the filter check calls MUCH easier to read and comprehend - Adding a default method isItemShown to the Condition interface makes for less duplicated code in the model classes
- ItemHelper inherited from ItemIdentifyHelper without any benefits - ItemHelper and ItemIdentifyHelper were used as wrappers - Only ItemHelper was used in a single place - Instead of using helpers, move the business logic of searching all sets for an item into the Context model class - Move code in Context that is related to sets next to getters/setters - Refactor the set code with streams
The Condition class is more of a filter (it's a very thin layer above). It might be refactored to become a union of Filter and Condition.
When changed from Date to Instant, the search and replace was also executed for method names, which is clearly not correct. This commit partially reverts some of the wrong changes.
XmlWriteException is now a subclass of XMLStreamException
Wrong comparison for the edge case leads to errors.
Bug had reported back the token value from the clients request instead of fetching the proper new response token value.
Changing the verb handler to use either the variant with a Request (single shot, no paged results) or with a ResumptionToken.Value (may have paged results) makes it more obvious what's needed and the handling code easier to understand when to use what. The methods in the VerbHandler parent class are no longer abstract but have a default implementation throwing an InternalOAIException to indicate the non-supported method. All inheriting classes and related methods (verification, filter creation) have been changed accordingly. DataProvider has been changed and added a TODO reminder that a token value provided by a client needs verification.
…page The paged results handlers need to provide a maximum length when asking for the resumption token to send with the response. This way, we can determine if a resumption token is needed at all (single pages may not have a <resumptionToken> element according to OAI-PMH spec)
Backward compatibility here.
poikilotherm
force-pushed
the
25-from-until-fixes
branch
from
faccc24 to
9ae4339
Compare
After the large refactoring, add more test cases and fix tests for handlers new method signatures.
When creating a resumption token from a request (as done in data provider), the token is not really empty - the offset is 0. This way, we can still check for empty tokens (which would be not allowed).
Due to the changed behaviour of the handlers throwing HandlerExceptions only, we need to throw the runtime exception InternalOAIException. This also makes more sense, as the exceptions does not happen because of a malicious user request (which is what we have been using the explicit exceptions for).
During building the Request instance via RequestBuilder, we need to skew the time of an until argument a bit. - For Day granularity, it's necessary to include the complete given day. - For Second granularity, it's necessary to add second to avoid exclusion of elements because of micro/nanosecond timestamps in databases. - For Lenient, we want to the complete day, too - that's why it's lenient. Adds a test for the skewing.
poikilotherm
force-pushed
the
25-from-until-fixes
branch
from
74816f0 to
443b6b4
Compare
- Verify the from and until timestamps of a request make sense when given. - If an until timestamp is given, tune it for inclusiveness as requested by the OAI-PMH spec
poikilotherm
force-pushed
the
25-from-until-fixes
branch
from
bacfbf5 to
bdc61c5
Compare
…and side-effect free
Member
Author
|
@landreev I think this is done. One TODO might remain: verification of a resumption token value encoded data. What do you think - should this be addressed in this PR or left as an exercise for later? |
|
Kudos, SonarCloud Quality Gate passed! |
Member
Author
|
It's Friday. Perfect time for shipping! |
Collaborator
A perfect candidate for an "exercise for later", yes. |
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
2 participants
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.
Closes #25
Closes #1
Closes #10
Closes #2
Besides the pure fix, this is also a larger refactoring of the classes, as the fix wasn't so easy to implement within the current flow of the data provider.