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

Fixed an issue where a server error was raised when downloading a PDF for an invalid article identifier #4344

Conversation

gamboz
Copy link
Collaborator

@gamboz gamboz commented Aug 6, 2024

URLs in the form /article/<identifier_type>/<identifier>/download/pdf/ call a view that tries to get the article using the identifier.

If the article does not exist (maybe because there was a typo in the identifier), or if the article has not yet been published, the view should return a 404.

This MR tries to fix a small bug that makes the clause above to fail badly when the article does not exists, because, ATM, the code checks if the non-existing article has been published 🙂

Unfortunately I wasn't able to replicate the issue on any online system where I tried. For instance, both the following URLs return a neat 404, but AFAICT the first should work and the second should give 500:

So, it's very possible I'm missing something here (i.e. please feel free to trash this MR).

@ajrbyers
Copy link
Member

ajrbyers commented Aug 6, 2024

@gamboz in this instance if article_object is none the second part of the if statement is not evaluated so it immediately raises the 404. You can test this by doing something like this in a python shell:

In [1]: article = None

In [2]: if not article or test:
   ...:     print('Article is None so even though test is never set it wont break.')
   ...: 
Article is None so even though test is never set it wont break.

In [3]: 

@gamboz
Copy link
Collaborator Author

gamboz commented Aug 6, 2024

@ajrbyers I agree, but the original code has if not article and... (with and, not or).

@mauromsl mauromsl requested a review from ajrbyers August 6, 2024 13:55
@ajrbyers ajrbyers merged commit 7c8ab27 into openlibhums:master Aug 6, 2024
@ajrbyers
Copy link
Member

ajrbyers commented Aug 6, 2024

@ajrbyers I agree, but the original code has if not article and... (with and, not or).

Haha, of course. Stupid me, I was looking at your new version XD

@mauromsl
Copy link
Member

mauromsl commented Aug 6, 2024

URLs in the form /article/<identifier_type>/<identifier>/download/pdf/ call a view that tries to get the article using the identifier.

If the article does not exist (maybe because there was a typo in the identifier), or if the article has not yet been published, the view should return a 404.

This MR tries to fix a small bug that makes the clause above to fail badly when the article does not exists, because, ATM, the code checks if the non-existing article has been published 🙂

Unfortunately I wasn't able to replicate the issue on any online system where I tried. For instance, both the following URLs return a neat 404, but AFAICT the first should work and the second should give 500:

* https://olh.openlibhums.org/article/doi/10.16995%2Folh.16937/download/pdf

* https://olh.openlibhums.org/article/doi/10.16995%2Folh.16937xxxxxxxxxxxxxxxx/download/pdf

So, it's very possible I'm missing something here (i.e. please feel free to trash this MR).

Provided examples do not reproduce it because they are missing a trailing slash, I can reproduce it like this: https://olh.openlibhums.org/article/id/999999999/download/pdf/

@mauromsl mauromsl changed the title When serving an article's PDF via the identifier URL, don't try to read an attribute from a None object Fixed an issue where a server error was raised when downloading a PDF for an invalid article identifier Aug 6, 2024
@gamboz gamboz deleted the bugfix/untracked-servepdf-nonexisting-article branch August 7, 2024 06:52
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.

3 participants