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

adding a method to be able to find requests matching a stub requests #2

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

anandsunderraman
Copy link

This is an attempt to add a find method to the client
This is for the verify behavior documented at http://wiremock.org/docs/verifying/

@@ -107,3 +108,36 @@ func (c *Client) ResetAllScenarios() error {

return nil
}

// FindFor finds requests that were made for a StubRule
func (c *Client) FindRequestsFor(stubRule *StubRule) (map[string]interface{}, error) {
Copy link
Author

Choose a reason for hiding this comment

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

for the sake of keeping this PR small returning a map[string]interface{} else we can work on modelling out the expected response as a struct

Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be great if we had a structure, at least requests []map[string]interface{}

Copy link
Author

Choose a reason for hiding this comment

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

ok will address this

stub_rule.go Outdated
//adding a separate MarshalJSON method for the request object
//as it is required to convert the request to JSON
func (r *request) MarshalJSON() ([]byte, error) {
req := map[string]interface{}{
Copy link
Collaborator

@walkerus walkerus Mar 4, 2021

Choose a reason for hiding this comment

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

It would be great if we could use the maximum number of the request fields.
Can we move all parsing of the request here and use the original structure here?

Copy link
Author

Choose a reason for hiding this comment

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

makes complete sense will address this

@@ -107,3 +108,36 @@ func (c *Client) ResetAllScenarios() error {

return nil
}

// FindFor finds requests that were made for a StubRule
func (c *Client) FindRequestsFor(stubRule *StubRule) (map[string]interface{}, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we only use the request?

Copy link
Author

Choose a reason for hiding this comment

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

sure let me try to refactor that

Copy link
Author

Choose a reason for hiding this comment

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

@walkerus the main reasoning to re-use StubRule for this request was because wire mock uses the similar structure for find request patterns. To quote the docs

The criteria part in the parameter to postRequestedFor() uses the same builder as for stubbing, so all of the same predicates are available

But if you feel strongly about using request alone then we have to create builders for request just like StubRule
So for every function that returns a StubRule like the following

func Get(urlMatchingPair URLMatcher) *StubRule {
	return NewStubRule(http.MethodGet, urlMatchingPair)
}

we have to create a corresponding function like below

func Get(urlMatchingPair URLMatcher) *request {
	return NewRequest(http.MethodGet, urlMatchingPair)
}

Let me know your thoughts around this

Copy link
Author

Choose a reason for hiding this comment

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

Oh or @walkerus did you mean we could build the StubRule as is but change the api alone to use request

func (c *Client) FindRequestsFor(request *request) (map[string]interface{}, error) {

but then the users need to know that they must use StubRule to build the request but when invoking the function they would need to know that they have to extract the request from the StubRule ??

Copy link
Author

Choose a reason for hiding this comment

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

Sorry for the multiple comments or if we could build a new set of builder for request and re-use them in StubRule like

func NewStubRule(method string, urlMatcher URLMatcher) *StubRule {
	return &StubRule{
		request: NewFindRequest(method, urlMatcher),
		response: response{
			status: http.StatusOK,
		},
	}
}

func NewFindRequest(method string, urlMatcher URLMatcher) request {
	return request{
			urlMatcher: urlMatcher,
			method:     method,
		}
}

// WithQueryParam adds query param and returns *StubRule
func (s *StubRule) WithQueryParam(param string, matcher ParamMatcherInterface) *StubRule {
	s.request.WithQueryParam(param, matcher)
	return s
}

func (r *request) WithQueryParam(param string, matcher ParamMatcherInterface) *request {
	if r.queryParams == nil {
		r.queryParams = map[string]ParamMatcherInterface{}
	}

	r.queryParams[param] = matcher
	return r
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks nice.
We can make the request importable structure (request -> Request) to be used separately from StubRule.
The request stubbing rules can already be implemented in the YAML file and someone can check if a specific request was received without initiating StubRule

Copy link
Author

Choose a reason for hiding this comment

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

thanks @walkerus will try to make changes accordingly

@anandsunderraman
Copy link
Author

@walkerus would appreciate if you are able to discuss some of the above comments

@walkerus
Copy link
Collaborator

walkerus commented Mar 15, 2021

@walkerus would appreciate if you are able to discuss some of the above comments

Of course! Sorry for the delay, I was on vacation.

@anandsunderraman
Copy link
Author

@walkerus would appreciate if you are able to discuss some of the above comments

Of course! Sorry for the delay, I was on vacation.

Ah no vacation responders on github LOL

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants