Skip to content

Fix redirects#152

Merged
umputun merged 1 commit intoumputun:masterfrom
ShoshinNikita:fix-redirect
May 14, 2023
Merged

Fix redirects#152
umputun merged 1 commit intoumputun:masterfrom
ShoshinNikita:fix-redirect

Conversation

@ShoshinNikita
Copy link
Contributor

@ShoshinNikita ShoshinNikita commented Nov 7, 2022

Before this change redirects didn't work because method Service.extendMapper didn't copy the value of URLMapper.RedirectType to the extended result.

To fix this we return the original URLMapper instead of creating a new one (it can also help to avoid similar bugs in the future). We can reuse URLMapper because it is passed by value.

@ShoshinNikita
fix redirects
006c6dd 
Before this change redirects didn't work because method `Service.extendMapper` didn't copy
the value of `URLMapper.RedirectType` to the extended result.

To fix this we return the original `URLMapper` instead of creating a new one (it can also help
to avoid similar bugs in the future). We can reuse `URLMapper` because it is passed
by value.
@ShoshinNikita
Copy link
Contributor Author

ShoshinNikita commented Nov 7, 2022 

2022/11/07 13:47:25 INFO  activate http proxy server on 127.0.0.1:49700
    proxy_test.go:66: 
        	Error Trace:	proxy_test.go:66
        	Error:      	Received unexpected error:
        	            	Get "http://127.0.0.1:49700/api/something": dial tcp 127.0.0.1:49700: connect: connection refused
        	Test:       	TestHttp_Do
--- FAIL: TestHttp_Do (0.06s)

I reproduced this fail (~1/50 test runs). It looks like the 10ms delay (app/proxy/proxy_test.go:58) sometimes is not long enough. I can increase it to 20ms, for example. Another option is to just re-run CI.

@ShoshinNikita
Copy link
Contributor Author

ShoshinNikita commented Nov 7, 2022 

2022/11/07 14:06:38.404 [INFO]  {logger/logger.go:134 logger.(*Middleware).Handler.func1.1} GET - /svc2/test - 127.0.0.1 - 127.0.0.1 - 404 (43) - 169.961115ms
main_test.go:81: 
      Error Trace:	main_test.go:81
      Error:      	Not equal: 
                    expected: 200
                    actual  : 404
      Test:       	Test_Main
main_test.go:84: 
      Error Trace:	main_test.go:84
      Error:      	"404 page not found\n" does not contain "echo echo 123"
      Test:       	Test_Main

app/main_test.go:28

"--static.rule=*,/svc2/(.*), https://echo.umputun.com/$1,https://feedmaster.umputun.com/ping",

It looks like https://echo.umputun.com/test doesn't work as expected anymore. I am not sure what caused this. Should I update expected status code and body?

@umputun
Copy link
Owner

umputun commented Nov 7, 2022

It looks like https://echo.umputun.com/test doesn't work as expected anymore

should be back

@ShoshinNikita
Copy link
Contributor Author

ShoshinNikita commented Nov 8, 2022

I think the decrease of coverage by 0.02% is not very critical. Especially, I believe it is caused by code removal.

umputun
umputun approved these changes May 14, 2023
View reviewed changes
Copy link
Owner

@umputun umputun left a comment

Choose a reason for hiding this comment

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

LGTM, thx a lot

@umputun umputun merged commit 2b92c11 into umputun:master May 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@umputun umputun umputun approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@ShoshinNikita @umputun