[HELP]: questions in desc #148

serranoio · 2025-02-17T18:41:13Z

mockMode does not work on redirected paths -

I needed paths to be redirected before requests are checked, so I took the functionality out of callApi() and put it on lines 143-144 in handle_request.go. This does not seem like good code, but I don't know if this breaks anything or not since there's no tests :(. But hey, it works and the tests pass. Is this ok?
For some reason, auth is dropped from the original header in my request when it comes FROM THE BROWSER - mockMode works when curling the endpoint with authentication added as headers, but when the request comes from the browser, it fails. So on line 145 in handle_request.go I added the header back and it works but this seems like a hack. Am I missing something? What am I missing? I cannot tell since there are no tests :(
Wiretap really needs tests. I really want to write a test like in wireap_service_test.go, but I just dont know how to get it to work(this test is commented out to show you that my changes to wiretap in 1.,2., work). Can you help me formulate the best testing strategy moving forward? What do I need to learn? I am seriously willing to beef wiretap tf up. It's going to need it after my planned changes (which I havent mentioned yet)

PS: extra changes which dont need review yet but I will describe them if you are curious-

requests made in mockMode now show up in the UI as mockMode.

2. I added a quick keyword filter at the top. Wanted to quickly search the httpTransactions.

mockMode does not work on redirected paths - 1. I needed paths to be redirected before requests are checked, so I took the functionality out of callApi() and put it on lines 143-144 in `handle_request.go`. This does not seem like good code, but I dont know if this breaks anything or not since there's no tests :(. But hey, it works and the tests pass. Is this ok? 2. For some reason, auth is dropped from the original header in my request when it comes FROM THE BROWSER - MockMode works when curling the endpoint with authentication added as headers, but when the request comes from the browser, it fails. So on line 145 in `handle_request.go` I added the header back, but this seems like a hack. Am I missing something? What am I missing? I cannot tell since there are no tests :( 3. Wiretap really needs tests. I really want to write a test like in `wireap_service_test.go`, but I just dont know how to get it to work. Any pointers? (this test is commented out to show you that my changes to wiretap in 1.,2., work)

daveshanley

This PR needs to broken down, I really don't know why the UI has been modified.

Can you remove the UI code? I am planning on stripping out 99% of the UI from wiretap and moving it to another repo - if you want to add UI changes, please do so in another PR so it's a composed set of PRs that build on top of each other.

Also, I don't think you have used gofmt as there are a bunch of whitespace changes.

daveshanley · 2025-02-19T23:28:20Z

daemon/api.go

-
-	// create a new request from the original request, but replace the path
-	wiretapConfig := configStore.(*shared.WiretapConfiguration)
+func (ws *WiretapService) rewritingPath(req *http.Request, wiretapConfig *shared.WiretapConfiguration) string {


Can you rename this to rewritingPath to checkForPathRewrite

daveshanley · 2025-02-19T23:30:56Z

daemon/handle_mock.go

@@ -58,6 +59,10 @@ func (ws *WiretapService) handleMockRequest(
 		}
 	}

+	const mocked = "this response has been mocked"


What is this?

This is a place holder for the future. Eventually I want to return to the UI what was used for mocking (examples, example, schema) and if there is a name, the name as well

daveshanley · 2025-02-19T23:31:20Z

daemon/handle_mock.go

@@ -58,6 +59,10 @@ func (ws *WiretapService) handleMockRequest(
 		}
 	}

+	const mocked = "this response has been mocked"
+	request.HttpResponseWriter.Header().Set("wiretap-mock", fmt.Sprintf(mocked))
+	header.Add("wiretap-mock", fmt.Sprintf(mocked))


What is this? why are we adding custom headers?

I wanted the UI to be able to differentiate between mocked data & non-mocked data. Since this is a 'simulated response to send up to the monitor UI, I thought that I could simply add wiretap specific data to the header

daveshanley · 2025-02-19T23:36:53Z

daemon/handle_request.go

-		ws.config.Logger.Info("MockMode enabled; skipping validation")
+		ws.rewritingPath(request.HttpRequest, config)
+		ws.rewritingPath(newReq, config)
+		request.HttpRequest.Header.Set("Authorization", apiRequest.Header.Get("Authorization"))


This is the wrong place to do this. I think there is something missing during the request cloning process https://github.com/pb33f/wiretap/blob/main/daemon/clone_request.go

Every request is cloned before being parsed because you can only ready the body once, therefore to allow the chain to continue, each time you read the body, you need to replace it with a fresh stream, because its dead.

I investigate what the browser is sending first and then try to fix this, because if the header is being set correctly via a cURL command, but failing with the browser - there is a failure to parse the header the browser is sending..

Start there, because this is a hack and we can't do this.

Thanks for the advice, thats what I thought

daveshanley · 2025-02-19T23:38:22Z

daemon/wiretap_service_test.go

@@ -0,0 +1,122 @@
+package daemon
+
+// this test does not work. I have no idea how to test the platform. It will take a decent amount of work to dig deep.


You don't need to test the platform, you need to test the logic, all can be mocked using the standard library.

mockedhttps://www.cloudbees.com/blog/testing-http-handlers-go

daveshanley · 2025-02-19T23:39:21Z

mock/mock_engine.go

+// https://swagger.io/docs/specification/v3_0/data-models/oneof-anyof-allof-not/
+// anyOf, oneOf, allOf, not
+// only supports v3 model, f the rest LOL
+func (rme *ResponseMockEngine) GetSchemaCombination(mediaType *v3.MediaType) {


I don't know what this is, delete please unless we need it.

Yeah so sorry. I did not mean for you to review the MR. This function was not finished. I only inteded for help.

The mockGenerator does not support anyOf, oneOf, allOf, not. I was going to implement it. The use case is that there exists different shapes of data that are returned from a singular endpoint (bad api design)

daveshanley · 2025-02-19T23:49:09Z

mockMode does not work on redirected paths -

I needed paths to be redirected before requests are checked, so I took the functionality out of callApi() and put it on lines 143-144 in handle_request.go. This does not seem like good code, but I don't know if this breaks anything or not since there's no tests :(. But hey, it works and the tests pass. Is this ok?

Why does it seem like bad code? All you did was shift logic around a little? The code is the same from what I can tell, you just moved things around a little?

For some reason, auth is dropped from the original header in my request when it comes FROM THE BROWSER - mockMode works when curling the endpoint with authentication added as headers, but when the request comes from the browser, it fails. So on line 145 in handle_request.go I added the header back and it works but this seems like a hack. Am I missing something? What am I missing? I cannot tell since there are no tests :(

I left a comment about this, I would investigate the request cloning process first and check out exactly what is going on with the browser sending headers that cannot be parsed, because if a cURL command works, but a browser request does not - then there is a major issue with parsing the header correctly, - because the logic is working.

Wiretap really needs tests. I really want to write a test like in wireap_service_test.go, but I just dont know how to get it to work(this test is commented out to show you that my changes to wiretap in 1.,2., work). Can you help me formulate the best testing strategy moving forward? What do I need to learn? I am seriously willing to beef wiretap tf up. It's going to need it after my planned changes (which I havent mentioned yet)

I think you need to take a step back and learn about http mock testing in go. You don't need to run the whole platform, you just need to use the standard library for testing HTTP, it's got a rich set of APIs for this.

https://github.com/pb33f/wiretap/blob/main/mock/mock_engine_test.go

There are limited tests in wiretap because the vast majority of logic lives outside this tool, it lives libopenapi and libopenapi-validator wiretap just adds a little control logic on top of it.

PS: extra changes which dont need review yet but I will describe them if you are curious-

requests made in mockMode now show up in the UI as mockMode.

2. I added a quick keyword filter at the top. Wanted to quickly search the httpTransactions.

I would really like to discuss the UI work first it, I am extremely fussy in this area :), it should come in as another PR.

mockMode does not work on redirected paths -

I needed paths to be redirected before requests are checked, so I took the functionality out of callApi() and put it on lines 143-144 in handle_request.go. This does not seem like good code, but I don't know if this breaks anything or not since there's no tests :(. But hey, it works and the tests pass. Is this ok?

For some reason, auth is dropped from the original header in my request when it comes FROM THE BROWSER - mockMode works when curling the endpoint with authentication added as headers, but when the request comes from the browser, it fails. So on line 145 in handle_request.go I added the header back and it works but this seems like a hack. Am I missing something? What am I missing? I cannot tell since there are no tests :(

Wiretap really needs tests. I really want to write a test like in wireap_service_test.go, but I just dont know how to get it to work(this test is commented out to show you that my changes to wiretap in 1.,2., work). Can you help me formulate the best testing strategy moving forward? What do I need to learn? I am seriously willing to beef wiretap tf up. It's going to need it after my planned changes (which I havent mentioned yet)

PS: extra changes which dont need review yet but I will describe them if you are curious-

requests made in mockMode now show up in the UI as mockMode.

2. I added a quick keyword filter at the top. Wanted to quickly search the httpTransactions.

serranoio · 2025-02-20T01:02:35Z

Hey, sorry! I forgot to mark this as a DRAFT mr. My intentions were to ask for help - code is not ready which is why the code looks like shit. Sorry about that 🙏

serranoio · 2025-02-20T01:28:15Z

Why does it seem like bad code? All you did was shift logic around a little? The code is the same from what I can tell, you just moved things around a little?

I am rewriting the path only for mockMode. I feel like we should only have to rewrite a path once for all of the cases.
But tbh, I dont have a clue, as I wouldn't know the blast radius of the change since theres no tests.

I left a comment about this, I would investigate the request cloning process first and check out exactly what is going on with the browser sending headers that cannot be parsed, because if a cURL command works, but a browser request does not - then there is a major issue with parsing the header correctly, - because the logic is working.
💯 💯 yes, thank you. This is a big help.

I think you need to take a step back and learn about http mock testing in go. You don't need to run the whole platform, you just need to use the standard library for testing HTTP, it's got a rich set of APIs for this.

Thank you, that makes sense. Will take a step back 🙏

https://github.com/pb33f/wiretap/blob/main/mock/mock_engine_test.go

There are limited tests in wiretap because the vast majority of logic lives outside this tool, it lives libopenapi and libopenapi-validator wiretap just adds a little control logic on top of it.
👍 💯

I would really like to discuss the UI work first it, I am extremely fussy in this area :), it should come in as another PR.

100% will put them in another PR. I do have several more changes - 2 new services planned. I'll propose them in issues.

daveshanley requested changes Feb 19, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HELP]: questions in desc #148

[HELP]: questions in desc #148

serranoio commented Feb 17, 2025 •

edited

Loading

daveshanley left a comment

daveshanley Feb 19, 2025

daveshanley Feb 19, 2025

serranoio Feb 20, 2025

daveshanley Feb 19, 2025

serranoio Feb 20, 2025

daveshanley Feb 19, 2025

serranoio Feb 20, 2025

daveshanley Feb 19, 2025

daveshanley Feb 19, 2025

serranoio Feb 20, 2025

daveshanley commented Feb 19, 2025

serranoio commented Feb 20, 2025

serranoio commented Feb 20, 2025

		@@ -0,0 +1,122 @@
		package daemon

		// this test does not work. I have no idea how to test the platform. It will take a decent amount of work to dig deep.

[HELP]: questions in desc #148

Are you sure you want to change the base?

[HELP]: questions in desc #148

Conversation

serranoio commented Feb 17, 2025 • edited Loading

daveshanley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daveshanley commented Feb 19, 2025

serranoio commented Feb 20, 2025

serranoio commented Feb 20, 2025

serranoio commented Feb 17, 2025 •

edited

Loading