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.

Sign up for GitHub

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

Jump to bottom

Issue 2177 - Fix skip token encoding #2197

Merged
merged 2 commits into from
Jul 10, 2020
Merged

Issue 2177 - Fix skip token encoding #2197

merged 2 commits into from
Jul 10, 2020

Conversation

YoannaAtanasova
Copy link
Contributor

@YoannaAtanasova YoannaAtanasova commented Jun 15, 2020 •
edited
Loading

Issues

This pull request fixes issue #2177

Description

With the changes in this PR we now encode all values that are put in the generated skiptoken.

Checklist (Uncheck if it is not completed)

  • Test cases added
  • Build and test with one-click build and test script passed

@ghost
Copy link

ghost commented Jun 15, 2020 •
edited by ghost
Loading

CLA assistant check
All CLA requirements met.

@xuzhg xuzhg requested a review from mikepizzo June 26, 2020 17:00
@xuzhg xuzhg added this to the 7.5 milestone Jun 26, 2020
@xuzhg xuzhg added the Ready for review Use this label if a pull request is ready to be reviewed label Jun 26, 2020
mikepizzo
mikepizzo requested changes Jun 26, 2020
View reviewed changes
Copy link
Member

@mikepizzo mikepizzo left a comment

Choose a reason for hiding this comment

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

Rather than pass in a boolean for whether or not to encode, which is going to be hard for the service to decide, can we just add logic to DefaultSkipTokenHandler to encode any date, string, or other values that may have suspect characters?

@YoannaAtanasova
Copy link
Contributor Author

YoannaAtanasova commented Jun 26, 2020 •
edited by mikepizzo
Loading

Rather than pass in a boolean for whether or not to encode, which is going to be hard for the service to decide, can we just add logic to DefaultSkipTokenHandler to encode any date, string, or other values that may have suspect characters?

I am not sure I understand you. I am not passing a boolean and the encoding is not conditional. #Resolved

@mikepizzo
Copy link
Member

Sorry; added my comment to the wrong PR. This one looks fine.

In reply to: 650314297 [](ancestors = 650314297)

mikepizzo
mikepizzo approved these changes Jul 7, 2020
View reviewed changes
Copy link
Member

@mikepizzo mikepizzo left a comment

Choose a reason for hiding this comment

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

:shipit:

@mikepizzo mikepizzo merged commit c49bfa5 into OData:master Jul 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mikepizzo mikepizzo mikepizzo approved these changes

@xuzhg xuzhg xuzhg approved these changes

Assignees
No one assigned
Labels
Ready for review Use this label if a pull request is ready to be reviewed
Projects
None yet
Milestone
7.5
Development

Successfully merging this pull request may close these issues.
3 participants
@YoannaAtanasova @mikepizzo @xuzhg