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

Pagination headers #1358

Merged
merged 14 commits into from
Apr 15, 2019
Merged

Pagination headers #1358

merged 14 commits into from
Apr 15, 2019

Conversation

kminehart
Copy link
Contributor

@kminehart kminehart commented Apr 14, 2019

Related issue

#1047

#1357

Proposed changes

To improve user experience, pagination needed to be improved without making backwards-incompatible changes to the JSON payload.

Now, for API requests where limit and offset are accepted, a Link header containing URLs for the "first", "next", "previous", and "last" pages is also sent. If any of those pages are not applicable, then the URL is not included.

Updated the Makefile to handle generating the migrations better. On a newer Go environment I wasn't able to create the migrations; go-bindata/go-bindata wasn't finding any of the SQL files. Instead of only pointing to the directory, I added ./... to recursively search for files to convert.

Checklist

  • I have read the contributing guidelines
  • I confirm that this pull request does not address a security vulnerability. If this pull request addresses a security
    vulnerability, I confirm that I got green light (please contact hi@ory.sh) from the maintainers to push the changes.
  • I signed the Developer's Certificate of Origin
    by signing my commit(s). You can amend your signature to the most recent commit by using git commit --amend -s. If you
    amend the commit, you might need to force push using git push --force HEAD:<branch>. Please be very careful when using
    force push.
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation within the code base (if appropriate)
  • I have documented my changes in the developer guide (if appropriate)

Further comments

I added tests for the manager functionality that was added, however I was unable to find a way to add headers for swagger build (in the comments in handler.go). I was hoping that the Link header generated / documented in the SDK, but I couldn't find a way to do it in go-swagger's documentation.

Any advice, @aeneasr?

Signed-off-by: Kevin Minehart <kmineh0151@gmail.com>
Signed-off-by: Kevin Minehart <kmineh0151@gmail.com>
Signed-off-by: Kevin Minehart <kmineh0151@gmail.com>
Signed-off-by: Kevin Minehart <kmineh0151@gmail.com>
Signed-off-by: Kevin Minehart <kmineh0151@gmail.com>
Signed-off-by: Kevin Minehart <kmineh0151@gmail.com>
Signed-off-by: Kevin Minehart <kmineh0151@gmail.com>
Signed-off-by: Kevin Minehart <kmineh0151@gmail.com>
Signed-off-by: Kevin Minehart <kmineh0151@gmail.com>
Signed-off-by: Kevin Minehart <kmineh0151@gmail.com>
Copy link
Member

@aeneasr aeneasr left a comment

Choose a reason for hiding this comment

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

Regarding Headers, I was only able to find this comment: go-swagger/go-swagger#70 (comment)

Maybe, for now, we just add this in the description portion?

client/manager.go Outdated Show resolved Hide resolved
Signed-off-by: Kevin Minehart <kmineh0151@gmail.com>
Signed-off-by: Kevin Minehart <kmineh0151@gmail.com>
Signed-off-by: Kevin Minehart <kmineh0151@gmail.com>
@aeneasr
Copy link
Member

aeneasr commented Apr 15, 2019

Awesome!

@aeneasr aeneasr merged commit f1ee77c into ory:master Apr 15, 2019
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.

2 participants