pkp / pkp-lib

The library used by PKP's applications OJS, OMP and OPS, open source software for scholarly publishing.
https://pkp.sfu.ca
GNU General Public License v3.0
307 stars 448 forks source link

Crossref status check errors in 3.3.0-x #6887

Closed AhemNason closed 3 years ago

AhemNason commented 3 years ago

This is a hard one to explain but something funny is happening and I can't quite figure it out. It started with a message from Crossref asking about a journal on 3.3.0-4 who claimed they were having an issue with their deposits.

are you familiar with OJS users querying for their submission results using either api.crossref.org/servlet/submissionDownload or doi.crossref.org/servlet/submissionDownload?

we got a question in from someone who says they’ve been using api.crossref.org/servlet/submissionDownload for the past five years or so, and it’s just recently stopped working. but we don’t have any record of that endpoint existing. the only query we know of that works for submission logs is against doi.crossref.org/servlet/submissionDownload

From what I can tell, all reference to submissionDownload in our code starts with api.* and not doi.*.

Furthermore, OJS 3.3.x users trying to pull status from Crossref are seeing two errors:

An ajax error:

[Thu Mar 25 11:29:08.721029 2021] [proxy_fcgi:error] [pid 6537:tid 47340214515456] [client 99.250.150.155:53586] AH01071: Got error 'PHP message: PHP Fatal error:  Uncaught GuzzleHttp\\Exception\\ClientException: Client error: `POST https://api.crossref.org/servlet/submissionDownload` resulted in a `404 Not Found` response:\n{"status":"error","message-type":"route-not-found","message-version":"1.0.0","message":"Route not found"}\n in /var/home/spiroski/oamjms.eu/www/lib/pkp/lib/vendor/guzzlehttp/guzzle/src/Exception/RequestException.php:113\nStack trace:\n#0 /var/home/spiroski/oamjms.eu/www/lib/pkp/lib/vendor/guzzlehttp/guzzle/src/Middleware.php(65): GuzzleHttp\\Exception\\RequestException::create(Object(GuzzleHttp\\Psr7\\Request), Object(GuzzleHttp\\Psr7\\Response))\n#1 /var/home/spiroski/oamjms.eu/www/lib/pkp/lib/vendor/guzzlehttp/promises/src/Promise.php(204): GuzzleHttp\\Middleware::GuzzleHttp\\{closure}(Object(GuzzleHttp\\Psr7\\Response))\n#2 /var/home/spiroski/oamjms.eu/www/lib/pkp/lib/vendor/guzzlehttp/promises/src/Promise.php(153): GuzzleHttp\\Promise\\Promise::callHandler(1, Object(GuzzleHttp\\Psr7\\Response), NULL)\n#3 /var/home/spiroski/oa...', referer: https://oamjms.eu/index.php/mjms/management/importexport/plugin/CrossRefExportPlugin

And this:

 Client error: `POST https://api.crossref.org/servlet/submissionDownload` resulted in a `404 Not Found` response:\n{"status":"error","message-type":"route-not-found","message-version":"1.0.0","message":"Route not found"}

This error looks like this: 2021-03-25 15 27 11

You might say to yourself, has something changed on Crossref's side? We do not think so, and here's why. While I cannot find any substantive changes to this api.*/doi.* code from 3.2, the status query does work in 3.2 (and from 3.1.2 and up until 3.3). This is what it should do when it's accurately reporting the error. These two journals, by the way, had the same validation error in their XML deposit.

2021-03-25 15 46 35

And, we're seeing in the code:

define('CROSSREF_API_STAUTS_URL', 'https://api.crossref.org/servlet/submissionDownload');

So, I'm not totally sure what's happening here. Crossref said that changing the endpoint to doi.crossref.org solved all the issues for the journal at hand.

Lastly, regardless of version, all valid deposits successfully registered and properly resolve. Our issues appear to be related specifically to the status check. I'm going to send this issue to Crossref folks also just in case anyone wants to weigh in. @israelcefrin has suggested that maybe there's a change to the way the system POSTs? Because the same failed status is returning different messages.

asmecher commented 3 years ago

This appears to have been separately discovered at https://github.com/pkp/ojs/pull/3082.

asmecher commented 3 years ago

@bozana, could you have a look at this?

It's possible that the change from Curl to Guzzle is related to this (https://github.com/pkp/pkp-lib/issues/6097).

bozana commented 3 years ago

PRs:

OJS: stable-3_3_0: https://github.com/pkp/ojs/pull/3086 stable-3_3_1: https://github.com/pkp/ojs/pull/3095 main: https://github.com/pkp/ojs/pull/3094 (only submodule update) crossref-ojs: https://github.com/pkp/crossref-ojs/pull/1

OPS: stable-3_3_0: https://github.com/pkp/ops/pull/142 stable-3_3_1: https://github.com/pkp/ops/pull/141 crossref-ops: https://github.com/pkp/crossref-ops/pull/1 main: https://github.com/pkp/ops/pull/140 (only submodule update)

bozana commented 3 years ago

Yes, this issue seems to be the same as here: https://github.com/pkp/ojs/pull/3082 And, yes, it came when the Guzzle was introduced.

@asmecher, could you take a look at the code changes? If OK, which stable branches should be fixed?

@AhemNason, would it be possible for you to test the changes above? Regarding the domain api. or doi. -- maybe we shall double check that with Crossref. I found this page https://crossref.gitlab.io/knowledge_base/docs/services/xml-deposit-synchronous-2/, where doi. are listed as production URLs, but below that table it says:

The Synchronous Deposit API is found at api.crossref.org/v2/deposit and supercedes the api.crossref.org/v1/deposit API, which is not synchronous. The API was originally created for PKP but is now used by Metadata Manager.

To deposit a submission make an HTTP POST request to the path

https://test.crossref.org/v2/deposits
https://api.crossref.org/v2/deposits

Thus, it seems like both would work and can be used...?

Thanks!

davidjb commented 3 years ago

@bozana See https://github.com/pkp/ojs/pull/3087 - I'd contacted and am working with Crossref around what changed. That PR changes the domain across; a separate but related issue to https://github.com/pkp/ojs/pull/3082.

Their official response is (currently) that api.crossref.org/servlet/submissionDownload wasn't ever a thing 😄 -- I've explained the situation so I'm very interested in seeing what they say.

bozana commented 3 years ago

Thanks a lot @davidjb! Yes, it would be good to just be 100% sure that we then do the right thing now. Should we then also use https://doi.crossref.org/v2/deposits instead of https://api.crossref.org/v2/deposits ?

davidjb commented 3 years ago

@bozana - just adjusting the submissionDownload URL at this point. Quoting Crossref’s staff:

https://api.crossref.org/v2/deposits is a special route that was created just for the OJS plugin, and specifically just for the automated deposits made via the OJS plugin. It shouldn't be used in any other situation

So seems fine for now. This API is still operational but https://github.com/CrossRef/rest-api-doc/blob/master/deprecated/deposit_api.md indicates it may be considered deprecated. Will ask Crossref for confirmation when they reply.

bozana commented 3 years ago

@asmecher, I will then also add that change of the domain name for the status check in the PR above (stable-3_3_0) and will then let you know...

bozana commented 3 years ago

This API points were introduced here: https://github.com/pkp/pkp-lib/issues/3803. It could theoretically be that nobody ever used that submissionDownload API point because: Some failure messages (like the validation errors, e.g. from the example image above) are saved immediately by getting the 403 response. And some failures have to be requested using that API point. With the change to Guzzle those 403 response failures were not saved, so the 'Failed' link was calling that API point instead. This way it was detected. If so, that would mean that nobody ever got those other kind of failurs... :-)

bozana commented 3 years ago

@asmecher, the URL is changed (part of the PR above). Could you please take a look? Thanks!

asmecher commented 3 years ago

That looks OK to me, @bozana. @davidjb, does https://github.com/pkp/ojs/pull/3086 check out for you?

davidjb commented 3 years ago

@asmecher Yep, looks good. This does the same as my URL-change PR in https://github.com/pkp/ojs/pull/3087 and expands/fixes the error checking in a more comprehensive way than in https://github.com/pkp/ojs/pull/3082 (so those both can be closed in lieu of this PR https://github.com/pkp/ojs/pull/3086).

The only thing I'd add is is whether the whole API response should be kept for later review if a deposit fails. At the moment, only the <msg> element is retained (ref) which is the most relevant info, but the whole XML response provides other submission/DOI/batch ID info and statistical data (example: https://github.com/pkp/pkp-lib/issues/6021#issue-641589727) that are probably worth retaining, though that'd depend on the audience (e.g. system admins or techs trying to debug with Crossref). Showing just the message in the error dialog is good though as it's a more succinct summary of the error.

bozana commented 3 years ago

I have no objections for saving the whole XML response, but I am also not sure about the audience -- what they would like to see i.e. if XML is understandable enough -- why not... :-)

davidjb commented 3 years ago

@bozana - in my case, if any our non-tech journal managers see any error, they'd be asking for my technical input 😄

Maybe it could be the best of both worlds -- the first line could be the message and then concatenated with a few line breaks, a heading like "Technical Details:" and the full XML. It's a tiny bit repetitive but would save a non-technical user trying to grok the XML, even if it is fairly readable.

bozana commented 3 years ago

@asmecher, @davidjb, I've added one more commit that saves the failure msg as well as the whole xml response (concatenated to the msg)...

asmecher commented 3 years ago

Thanks, @davidjb and @bozana! @bozana, please go ahead with the merge and I'll get the next 3.3.0 build out this week with the fix included.

davidjb commented 3 years ago

Just noticed https://github.com/pkp/ojs/pull/3091 for moving crossref out to its own repo https://github.com/pkp/crossref-ojs; I'm sure everyone's aware, just don't want these fixes to get lost in the transition.

Thanks @bozana and @asmecher for your awesome work.

bozana commented 3 years ago

The PRs for stable-3_3_0 and stable 3_3_1 are merged. I will wait for https://github.com/pkp/ojs/pull/3091 to be closed to then port the changes to main. Thanks a lot @davidjb and @asmecher!

asmecher commented 3 years ago

@bozana, I think this is ready for porting to main!

bozana commented 3 years ago

Oh, I didn't notice that there is crossref plugin for OPS, so first now I added the changes for the OPS stable branches :-( That's why I changed the milestone to 3.3.0-6. When tests pass I will merge and close the issue.