Skip to content

Fix #8817 #8820 Improve CookieRequestCache #8818

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
Closed
wants to merge 1 commit into from
Closed

Fix #8817 #8820 Improve CookieRequestCache #8818

wants to merge 1 commit into from

Conversation

majian159
Copy link
Contributor

@majian159 majian159 commented Jul 8, 2020 •
edited
Loading

fixes #8817 #8820

@majian159 majian159 changed the title Fix RequestCache throw exception and error redirect Fix CookieRequestCache throw exception and error redirect Jul 8, 2020
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jul 8, 2020
@majian159 majian159 changed the title Fix CookieRequestCache throw exception and error redirect Fix #8817 #8820 Improve CookieRequestCache Jul 9, 2020
@zeeshanadnan
Copy link
Contributor

@majian159 thanks for the fix. I will look into it as well. The build is failing due to the tests. You have to update the tests as well of CookieRequestCacheTest. @eleftherias @majian159 let me know if i can be of any help.

eleftherias
eleftherias suggested changes Jul 9, 2020
View reviewed changes
Copy link
Contributor

@eleftherias eleftherias left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @majian159 and thanks for the help @zeeshanadnan!

@majian159 Please also add tests for the bugs reported. This will ensure they do not show up again.

@eleftherias eleftherias added in: web An issue in web modules (web, webmvc) type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged labels Jul 9, 2020
@majian159
Copy link
Contributor Author

majian159 commented Jul 10, 2020 •
edited
Loading

@majian159 thanks for the fix. I will look into it as well. The build is failing due to the tests. You have to update the tests as well of CookieRequestCacheTest. @eleftherias @majian159 let me know if i can be of any help.

Thanks, i updated the unit test, if you can please help me review.

@majian159 majian159 requested a review from eleftherias July 10, 2020 03:06
@majian159
Copy link
Contributor Author

majian159 commented Jul 10, 2020 •
edited
Loading

Thanks for the PR @majian159 and thanks for the help @zeeshanadnan!

@majian159 Please also add tests for the bugs reported. This will ensure they do not show up again.

I perfected unit tests, please check.

@zeeshanadnan
Copy link
Contributor

zeeshanadnan commented Jul 12, 2020 •
edited
Loading

@majian159 thanks for the fix. I will look into it as well. The build is failing due to the tests. You have to update the tests as well of CookieRequestCacheTest. @eleftherias @majian159 let me know if i can be of any help.

Thanks, i updated the unit test, if you can please help me review.

Thanks @majian159 for taking the time and responsibility to fix this. Adding an extra test for cache in not removed by mistake for non matching request would be a good idea. What do you think?

@majian159
Copy link
Contributor Author

@majian159 thanks for the fix. I will look into it as well. The build is failing due to the tests. You have to update the tests as well of CookieRequestCacheTest. @eleftherias @majian159 let me know if i can be of any help.

Thanks, i updated the unit test, if you can please help me review.

Thanks @majian159 for taking the time and responsibility to fix this. Adding an extra test for cache in not removed by mistake for non matching request would be a good idea. What do you think?

I added the unit test'notMatchingRequestWhenRequestNotContainsSavedRequestCookie' please check.

majian159
majian159 commented Jul 14, 2020
View reviewed changes
Copy link
Contributor Author

@majian159 majian159 left a comment

Choose a reason for hiding this comment

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

add unit test.

majian159
majian159 commented Jul 14, 2020
View reviewed changes
Copy link
Contributor Author

@majian159 majian159 left a comment

Choose a reason for hiding this comment

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

complete

majian159
majian159 commented Jul 14, 2020
View reviewed changes
Copy link
Contributor Author

@majian159 majian159 left a comment

Choose a reason for hiding this comment

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

complete

@majian159
Copy link
Contributor Author

Thanks for the PR @majian159 and thanks for the help @zeeshanadnan!

@majian159 Please also add tests for the bugs reported. This will ensure they do not show up again.

I perfected unit tests, please check.

@eleftherias

@eleftherias
Copy link
Contributor

@majian159 Please rebase against the master branch.
There are some errors unrelated to your changes that are causing the build to fail.

@majian159 majian159 closed this Jul 14, 2020
@majian159 majian159 reopened this Jul 14, 2020
@majian159
Copy link
Contributor Author

majian159 commented Jul 14, 2020 •
edited
Loading

@majian159 Please rebase against the master branch.

There are some errors unrelated to your changes that are causing the build to fail.

complete. @eleftherias

majian159
majian159 commented Jul 14, 2020
View reviewed changes
Copy link
Contributor Author

@majian159 majian159 left a comment

Choose a reason for hiding this comment

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

review

eleftherias
eleftherias suggested changes Jul 15, 2020
View reviewed changes
Copy link
Contributor

@eleftherias eleftherias left a comment

Choose a reason for hiding this comment

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

Thanks for the updates @majian159! I have left some comments inline.

@majian159
Improve request matching logic, repair request cache deleted by mistake.
bd819e4 
Fix RequestCache throw exception and error redirect.
Fix and improve CookieRequestCache unit tests.
Add notMatchingRequestWhenRequestNotContainsSavedRequestCookie unit test.
@majian159
Copy link
Contributor Author

I fixed these issues, please check.
@eleftherias

majian159
majian159 commented Jul 21, 2020
View reviewed changes
Copy link
Contributor Author

@majian159 majian159 left a comment

Choose a reason for hiding this comment

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

complete

@eleftherias
Copy link
Contributor

Thanks @majian159! This is now merged into master via 41f26b7 and fb936e2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eleftherias eleftherias eleftherias requested changes

Assignees

@eleftherias eleftherias

Labels
in: web An issue in web modules (web, webmvc) type: bug A general bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

use CookieRequestCache something went wrong
4 participants
@majian159 @zeeshanadnan @eleftherias @spring-projects-issues