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

Fix PaginationToken Circe codecs #752

Merged
merged 3 commits into from
May 25, 2021

Conversation

pomadchin
Copy link
Contributor

@pomadchin pomadchin commented May 20, 2021

Overview

This PR fixes PaginationToken circe codecs. Without it post requests with the passed next token doesn't work. I discovered this issue in terms of stac-utils/stac4s#327

Checklist

  • New tests have been added or existing tests have been modified

@pomadchin pomadchin requested a review from jisantuc May 20, 2021 03:31
@pomadchin pomadchin force-pushed the fix/pagination-token branch from e47584e to 7224888 Compare May 20, 2021 03:44
@jisantuc
Copy link
Contributor

jisantuc commented May 20, 2021

Could this get a test? I think it wouldn't be too tough to add similar to here --

https://github.com/azavea/franklin/blob/master/application/src/test/scala/com/azavea/franklin/api/services/SearchServiceSpec.scala#L73-L82

The testing procedure would be like:

  • generate two items
  • send a POST search request with no filters with a limit of 1
  • ensure that you can use the next link to send another valid POST request

@pomadchin
Copy link
Contributor Author

@jisantuc ok, will push it, but I want to admit, that because codecs are incorrect test would work anyway. That's due to the fact that both default encoder and decoder are incorrect (they encode PaginationToken into a JSON object not into a string).

val result2 = result1
.flatMap {
_.traverse { r =>
/** This line intentionally decodes next token [[String]] into [[PaginationToken]] */
Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a pretty important comment

Copy link
Contributor

@jisantuc jisantuc left a comment

Choose a reason for hiding this comment

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

both default encoder and decoder are incorrect (they encode PaginationToken into a JSON object not into a string).

I don't think that's quite right unless you're talking about the circe codecs. The codec for the query param decodes from a b64 encoded string --

implicit val codecPaginationToken: Codec.PlainCodec[PaginationToken] =
Codec.string.mapDecode(PaginationToken.decPaginationToken)(PaginationToken.encPaginationToken)

I think actually the derived decoders just shouldn't be there, since we never want to output / consume the JSON representation with fields and want it to stay opaque for clients. Maybe I never really understood what you were saying the problem was, but if the circe codec used the same encPaginationToken and decPaginationToken methods, I think everything would be consistent?

@pomadchin
Copy link
Contributor Author

@jisantuc not PaginationToken was the issue, but more an incorrect decoding of the SearchFilters.

@pomadchin pomadchin requested a review from jisantuc May 25, 2021 16:21
Copy link
Contributor

@jisantuc jisantuc left a comment

Choose a reason for hiding this comment

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

🚀 it took me a minute to understand the problem, but I've confirmed now that you get the same results with:

  • posting search filters with the b64 encoded pagination token in the next field
  • posting empty search filters to the url with the next qp set

I also confirmed that the POST with the b64 encoded token to the AVIRIS Franklin deployment is a 400

@pomadchin pomadchin merged commit b28fe1a into azavea:master May 25, 2021
@pomadchin pomadchin deleted the fix/pagination-token branch May 25, 2021 19:02
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