Allow setting different responses per request #31 #102

infinite-spectrum · 2021-06-22T15:56:40Z

issue: #31

Following is the summary of the changes done in the PR so far:

As part of this PR, I've introduced a new way to handle Random/Repeatable responses.
I've used burst terminology for repeatable responses.

Changed Items:

Changed response field in an imposter to responses (array of response)

Added Items:

Added burst field in response schema under imposter schema
- this new field has a +integer value, which represents no of times a response should be repeated.
- valid only in BURST mode, ignored in RANDOM mode
- in BURST mode if ignored/missing, the default value is 1
Added responseMode field in request schema under imposter schema
- this new field can have two string values RANDOM or BURST
- default value is RANDOM
- any other values will be ignored and default values will be selected instead
- if the field is missing/ignored, the default value will be considered

Input required:

Migration strategy for existing imposter schema to new schema (response -> array of response)
Determining default value for responseMode which is currently RANDOM

* Adding logic to have multiple responses for similar request * Changed imposters json and yaml schema to have array of response instead just response * Fixing tests according to changes * Adding TODOs

* Updating TODOs

* Modularizig code * Added logic for random responses if burst field is absent

* Changing implementation of random mode and burst mode selection

* Updating TODO

* Updating exisiting and adding new documentation portion as per the feature.

* Ignoring negative or 0 value in the burst mode. Using 1 value in that case.

* Fixing and maintaining name consistencey in the response_handler.go file

* Updating TODOs

* Adding unit tests for repeatable responses * Removing TODO file * Changing name of a function

* Fixing comments

* Fixing linting errors * Fixing edge case of having no responses available * Added test case for no response available, fixing tests

* Adding test for no response and minor fixes

joanlopez · 2021-07-01T07:47:49Z

README.md

+    - `RANDOM` and `BURST` are valid values 
+    - `RANDOM` is default if field is not provided or any other value is provided.
+2. `response` -> `burst` (applicable only in `BURST` mode, ignored in)
+    - +ve integer are valid values.


What do you mean by +ve? 🤔

Here, +ve means positive. I'll change it to be positive instead of +ve.

joanlopez

Hey @infinite-spectrum,

Thanks a lot for your ideas & code contributions. I'll need more time to test the new features and to do an exhaustive code review. However, after a quick review, I have two things to begin the discussion with:

What do you think about moving the responseMode to the imposter's level or to make the response field an object with mode and payloads fields within?
Have you tested your implementation with concurrent requests? I might be wrong (as I said I had no time to do some tests neither look at it in-detail), but looks like you're increasing the requests counter with no protection against concurrent requests (ergo potential data races).

Any thoughts about both topics? Thanks!! 😊

infinite-spectrum · 2021-07-05T12:59:08Z

@joanlopez

What do you think about moving the responseMode to the imposter's level or to make the response field an object with mode and payloads fields within?

>> I'd go with 2nd option rather than the 1st one.

Have you tested your implementation with concurrent requests? I might be wrong (as I said I had no time to do some tests neither look at it in-detail), but looks like you're increasing the requests counter with no protection against concurrent requests (ergo potential data races).

>> Nope, this is a miss from my end. I'll incorporate this one.

joanlopez · 2021-07-05T21:30:00Z

What do you think about moving the responseMode to the imposter's level or to make the response field an object with mode and payloads fields within?

I'd go with 2nd option rather than the 1st one.

I'm still wondering about how big that change is. In comparison with tools like Mountebank, in Killgrave is much simpler to define your imposters, so with this kind of change (forcing the user to define an array, etc) we're losing the added value that Killgrave brings into the ecosystem.

That being said, do you think it could work to either:

Make the imposters parser work with both schemas (older and newer).
Base this new feature in a completely new field (response vs responses or similar).

What do you think? cc/ @aperezg

Have you tested your implementation with concurrent requests? I might be wrong (as I said I had no time to do some tests neither look at it in-detail), but looks like you're increasing the requests counter with no protection against concurrent requests (ergo potential data races).

Nope, this is a miss from my end. I'll incorporate this one.

No worries at all, that's why code reviews are for. Feel free to ping me if you have any question related with that.

Again, thanks a lot for your engagement with the project and your valuable contributions 🙏🏻

Using mutex lock for response handler's state change

joanlopez · 2021-07-14T09:06:05Z

internal/server/http/response_handler.go

@@ -54,14 +59,20 @@ func (rh *ResponseHandler) fillDefaults(imposter *Imposter) {
 		rh.counter = 1
 		rh.currentInd = 0
 	}
+
+	rh.mutex = new(sync.Mutex)


AFAIK, there's no need to make the rh.mutex a pointer neither to initialize it since rh is already a pointer to a ResponseHandler. See this example from the Go Tour. Thanks!

aperezg · 2021-10-12T21:58:53Z

hello @infinite-spectrum will you continue with this PR? seems very good contibution so I hope to see this finish :D

Hadling concurrent requests for repeating responses

infinite-spectrum · 2021-12-07T12:51:29Z

@aperezg, sorry for the late submission. I've fixed the code for concurrent requests. Please review and let me know any suggestions/issues.

infinite-spectrum · 2021-12-07T13:00:52Z

@aperezg, we have to discuss one question - How to deal with the schema change? (responses instead of response in the imposter file.)

Here are few ideas:

Make current code to work with both imposter schemas. (@joanlopez's idea)
Write a one-time utility to convert imposter from previous version to the current one

Any other ideas would be appreciated.
Thank you

aperezg · 2022-02-22T22:53:23Z

Hello @infinite-spectrum, sorry for the late response 🙏 so for continue with the PR, I think that the best idea would be that joan said, support the responses and response :)

apurvasaraiya added 5 commits June 21, 2021 16:37

different-response-repeats:

1f72212

* Adding logic to have multiple responses for similar request * Changed imposters json and yaml schema to have array of response instead just response * Fixing tests according to changes * Adding TODOs

different-response-repeats:

ee2c389

* Updating TODOs

different-response-repeats:

27d49ac

* Modularizig code * Added logic for random responses if burst field is absent

different-response-repeats:

a7db874

* Changing implementation of random mode and burst mode selection

different-response-repeats:

6cf3732

* Updating TODO

infinite-spectrum mentioned this pull request Jun 22, 2021

Allow setting different responses per request (repeats) #31

Open

apurvasaraiya added 4 commits June 23, 2021 12:12

different-response-repeats:

682e0b1

* Updating exisiting and adding new documentation portion as per the feature.

different-response-repeats:

5afaa87

* Ignoring negative or 0 value in the burst mode. Using 1 value in that case.

different-response-repeats:

7471992

* Fixing and maintaining name consistencey in the response_handler.go file

different-response-repeats:

4b0048d

* Updating TODOs

infinite-spectrum changed the title ~~Allow setting different responses per request #68~~ Allow setting different responses per request #31 Jun 24, 2021

different-response-repeats:

88c7493

* Adding unit tests for repeatable responses * Removing TODO file * Changing name of a function

infinite-spectrum marked this pull request as ready for review June 24, 2021 13:53

apurvasaraiya added 4 commits June 24, 2021 19:30

different-response-repeats:

53af7af

* Fixing comments

different-response-repeats:

26583e7

* Fixing comments

different-response-repeats:

06551d5

* Fixing linting errors * Fixing edge case of having no responses available * Added test case for no response available, fixing tests

different-response-repeats:

15c99bd

* Adding test for no response and minor fixes

joanlopez reviewed Jul 1, 2021

View reviewed changes

joanlopez requested a review from aperezg July 1, 2021 07:55

different-response-repeats:

c94d8fa

Using mutex lock for response handler's state change

joanlopez reviewed Jul 14, 2021

View reviewed changes

apurvasaraiya and others added 2 commits December 7, 2021 18:11

diff-res-per-req-repeats:

01dbd8c

Hadling concurrent requests for repeating responses

Merge branch 'main' into diff-res-per-req-repeats

dff77dc

infinite-spectrum requested a review from joanlopez December 7, 2021 12:54

infinite-spectrum closed this by deleting the head repository Jul 5, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow setting different responses per request #31 #102

Allow setting different responses per request #31 #102

infinite-spectrum commented Jun 22, 2021 •

edited

Loading

joanlopez Jul 1, 2021

infinite-spectrum Jul 2, 2021 •

edited

Loading

joanlopez left a comment

infinite-spectrum commented Jul 5, 2021 •

edited

Loading

joanlopez commented Jul 5, 2021 •

edited

Loading

joanlopez Jul 14, 2021

aperezg commented Oct 12, 2021

infinite-spectrum commented Dec 7, 2021

infinite-spectrum commented Dec 7, 2021

aperezg commented Feb 22, 2022

Allow setting different responses per request #31 #102

Allow setting different responses per request #31 #102

Conversation

infinite-spectrum commented Jun 22, 2021 • edited Loading

joanlopez Jul 1, 2021

Choose a reason for hiding this comment

infinite-spectrum Jul 2, 2021 • edited Loading

Choose a reason for hiding this comment

joanlopez left a comment

Choose a reason for hiding this comment

infinite-spectrum commented Jul 5, 2021 • edited Loading

joanlopez commented Jul 5, 2021 • edited Loading

joanlopez Jul 14, 2021

Choose a reason for hiding this comment

aperezg commented Oct 12, 2021

infinite-spectrum commented Dec 7, 2021

infinite-spectrum commented Dec 7, 2021

aperezg commented Feb 22, 2022

infinite-spectrum commented Jun 22, 2021 •

edited

Loading

infinite-spectrum Jul 2, 2021 •

edited

Loading

infinite-spectrum commented Jul 5, 2021 •

edited

Loading

joanlopez commented Jul 5, 2021 •

edited

Loading