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

[bug] fix http template dict empty error #163

Merged
merged 4 commits into from
Aug 4, 2021
Merged

[bug] fix http template dict empty error #163

merged 4 commits into from
Aug 4, 2021

Conversation

benja-wu
Copy link
Contributor

@benja-wu benja-wu commented Aug 4, 2021

Problem

  1. HTTPTemplate doesn't work as expected in HTTPPipeline

Details

  • As the former design, HTTPTempalte will save one filter's request, and the response modified by it into context's template dictionary if necessary(If it can find the template in filter's spec), the flow is a sequence. That means the flow will look like
1. save http request to be handle by filter1
2. fitler1 handle
3. save http response modified by filter1
4. save http request to be handle by filter2
5. filter2 handle
6. save http response modified by filter2
....
  • After we introduced responsibility chain into HTTPPipline, the flow logic had been changed from sequence to stack. That means the flow looks like
1. save http request to be handle by filter1
2. filter1 handle
3. save http request to be handle by filter2
4. filter2 handle
5. save http response modified by filter2
6. save http response modified by filter1
....
  • This bug fixing is aiming to correct the HTTPTemplate saving flow back to the sequence type.

@benja-wu benja-wu added the bug Something isn't working label Aug 4, 2021
@benja-wu benja-wu added this to the v1.2.0 milestone Aug 4, 2021
Copy link

@megaeasex megaeasex left a comment

Choose a reason for hiding this comment

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

[TASK:easegress-pr-test SUCCESS]megaease/easegress Pull Request 163 Deploy Test Success

@codecov-commenter
Copy link

codecov-commenter commented Aug 4, 2021

Codecov Report

Merging #163 (0c28551) into main (c32e42c) will increase coverage by 0.16%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #163      +/-   ##
==========================================
+ Coverage   57.81%   57.98%   +0.16%     
==========================================
  Files          38       38              
  Lines        3658     3658              
==========================================
+ Hits         2115     2121       +6     
+ Misses       1437     1433       -4     
+ Partials      106      104       -2     
Impacted Files Coverage Δ
pkg/util/jmxtool/common.go 72.97% <0.00%> (+4.05%) ⬆️
pkg/util/jmxtool/agent_controller.go 54.09% <0.00%> (+4.91%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c32e42c...0c28551. Read the comment docs.

Copy link
Collaborator

@localvar localvar left a comment

Choose a reason for hiding this comment

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

line 492-495 should be removed.

@benja-wu
Copy link
Contributor Author

benja-wu commented Aug 4, 2021

line 492-495 should be removed.

My bad, let me update it

Copy link

@megaeasex megaeasex left a comment

Choose a reason for hiding this comment

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

[TASK:easegress-pr-test SUCCESS]megaease/easegress Pull Request 163 Deploy Test Success

Copy link

@megaeasex megaeasex left a comment

Choose a reason for hiding this comment

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

[TASK:easegress-pr-test SUCCESS]megaease/easegress Pull Request 163 Deploy Test Success

pkg/object/httppipeline/httppipeline.go Outdated Show resolved Hide resolved
pkg/object/httppipeline/httppipeline.go Outdated Show resolved Hide resolved
pkg/object/httppipeline/httppipeline.go Outdated Show resolved Hide resolved
pkg/object/httppipeline/httppipeline.go Outdated Show resolved Hide resolved
Copy link

@megaeasex megaeasex left a comment

Choose a reason for hiding this comment

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

[TASK:easegress-pr-test SUCCESS]megaease/easegress Pull Request 163 Deploy Test Success

@xxx7xxxx xxx7xxxx merged commit 405c92e into easegress-io:main Aug 4, 2021
@benja-wu benja-wu deleted the httptemplate branch August 10, 2021 06:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants