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

Set earliest/latest timestamp to current time for empty results #33

Merged
merged 3 commits into from
Apr 18, 2022

Conversation

ryanfb
Copy link
Contributor

@ryanfb ryanfb commented Nov 18, 2021

Fixes #26.

This falls back to a timestamp generated from the current time if there are no Solr documents or timestamps present.

According to the OAI-PMH spec, the earliestDatestamp element this will be used in has the following criteria:

earliestDatestamp : a UTCdatetime that is the guaranteed lower limit of all datestamps recording changes, modifications, or deletions in the repository. A repository must not use datestamps lower than the one specified by the content of the earliestDatestamp element. earliestDatestamp must be expressed at the finest granularity supported by the repository.

Since in this case there are no records with datestamps, using the current timestamp should be fine, and result in no differences from the case where a repository has a set of records with an earliest datestamp and then later has a new record added later on with an earlier datestamp (since the earliestDatestamp will update accordingly).

I've also made a backport onto the v6.1.1 tag here: https://github.com/ryanfb/blacklight_oai_provider/tree/default-earliest-datestamp-6.1.1-backport

@barmintor
Copy link
Collaborator

A couple of things:

  1. My read of the spec is that this scenario should result in <error code="noRecordsMatch"/>, and no Identify at all, right?
  2. These changes will trip CI on a linter basis, because the syntax is not compatible with Ruby 2.1 (which is the Blacklight 7.0-7.14 spec). So we would need an intermediate release on the 7.x line here?
  3. We need some tests - obviously the nil pointer is not what we want, but (per above) I'm not sure that providing Time.now is the right information to send back to clients.

@barmintor
Copy link
Collaborator

To be clear, I think we should fix this and get the 7.x gems released! This is impacting some of our apps as well, and I'll pitch in to get this done.

@ryanfb
Copy link
Contributor Author

ryanfb commented Mar 9, 2022

Interesting…my read of the spec is that Identify cannot return noRecordsMatch (the only available error code for it is badArgument), however, it must include an earliestDatestamp with the specified constraints. NB that with this patch applied, both ListRecords and ListIdentifiers will correctly return a noRecordsMatch error response for an empty repository, instead of an HTTP 500. Should Identify for an empty repository just explicitly return HTTP 500 with an empty body without triggering an exception?

In any case, I'm happy to backport syntax to Ruby 2.1 compatibility, and try to add some tests as well.

@barmintor
Copy link
Collaborator

Oh, I see what you mean re: Identify. Hmm. It makes me a little uncomfortable - it seems like it invites some weird client behavior when/if records are added - but maybe it's fine? Or at least fine enough that it could change with a patch release if we get the tests and linter issues sorted?

@barmintor
Copy link
Collaborator

Coming back to this since it had been some time: I fixed up the Ruby 2.1 syntax (and some other style issues in the spec), split off the legacy specs that actually use Solr into an integration suite, and added unit specs for the specific change in this PR.

@barmintor barmintor requested a review from dkinzer April 18, 2022 13:29
@dkinzer
Copy link
Member

dkinzer commented Apr 18, 2022

@barmintor I approved the PR, but I do not have write access to this repo, so I cannot merge it.

@ryanfb
Copy link
Contributor Author

ryanfb commented Apr 18, 2022

@barmintor Thanks for revisiting this and fixing it up!

@barmintor barmintor merged commit 557c4d3 into projectblacklight:master Apr 18, 2022
@barmintor
Copy link
Collaborator

This, it turns out, was a regression: 5713d30

But we clearly needed the 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.

Nil Pointer Exception with empty results
3 participants