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

Response consistency fixes #50

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Response consistency fixes #50

wants to merge 3 commits into from

Conversation

jacobsandersen
Copy link
Collaborator

@jacobsandersen jacobsandersen commented Jun 19, 2021

There was a regression in #38 that caused nonexistent resources and authors to show a normal response with every field nulled out. This patch fixes that.

Furthermore, when using the getResourcesByAuthor, getResourceUpdates, and listResources routes if you used a negative page then you would get a 404. However, if you used a page too big you would get an empty array. Also, if the user or resource did not exist (in the case of the former two routes), you would get an empty array. For all the cases you would typically get an empty array, you now get a 404.

To achieve the latter part, there are two new database checks using optimized row existence queries (i.e. making use of EXISTS, not COUNT).

This fixes #48.
This fixes #53.

@jacobsandersen jacobsandersen requested a review from md-5 June 19, 2021 16:34
@jacobsandersen jacobsandersen self-assigned this Jun 19, 2021
@jacobsandersen
Copy link
Collaborator Author

@marc-maurer would you be willing to give this a look-see? :) Thank you!

Copy link

@M4rcDev M4rcDev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

@md-5
Copy link
Member

md-5 commented Jun 19, 2021

Seems fine

@jacobsandersen
Copy link
Collaborator Author

Going to tackle #53 in this as well, changing to WIP

@jacobsandersen jacobsandersen marked this pull request as draft June 24, 2021 18:49
@md-5
Copy link
Member

md-5 commented Dec 28, 2021

I realise this probably fixed my last 3 commits....

@jacobsandersen
Copy link
Collaborator Author

That's ok, I'll pull in your stuff when I get a moment to get back to this. No biggie

@md-5
Copy link
Member

md-5 commented Dec 28, 2021

You can probably just revert all 3 when trying to merge.

The reason I picked up on them is we upgraded to php 7.4 and that shows errors with the nulled / broken responses

…pull up resources by user/updates by resource; also check if listResources has any results and if the array is empty (i.e. no more results) then send a 404 instead of a plain empty array off to infinity; closes #48.
@jacobsandersen jacobsandersen marked this pull request as ready for review November 4, 2022 07:21
@jacobsandersen
Copy link
Collaborator Author

It only took me 498 days but I have now fixed #53 and put it in this PR as originally intended.

@marc-maurer If you're still available could you review this again? Nothing has really changed since your last review except for Resource.php.

@M4rcDev
Copy link

M4rcDev commented Nov 4, 2022

It only took me 498 days but I have now fixed #53 and put it in this PR as originally intended.

@marc-maurer If you're still available could you review this again? Nothing has really changed since your last review except for Resource.php.

I will take a look later today :)

Copy link

@M4rcDev M4rcDev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had unfourtantly no chance to test the SQL Queries itself but it looks good to me.

Thank you for completing this pull request! :)

src/support/Database.php Outdated Show resolved Hide resolved
src/support/Database.php Outdated Show resolved Hide resolved
src/support/Database.php Outdated Show resolved Hide resolved
src/support/Database.php Outdated Show resolved Hide resolved
src/object/Resource.php Outdated Show resolved Hide resolved
src/object/Resource.php Show resolved Hide resolved
@jacobsandersen
Copy link
Collaborator Author

Great! Thank you Marc, I appreciate it. I'll need to take a look later and ensure I'm using strict equality where necessary. I am sure there are other areas where I am using ==.

I have made the changes you suggested

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants