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

Feature/query parameters #39

Merged
merged 6 commits into from
Aug 29, 2016

Conversation

jwir3
Copy link
Contributor

@jwir3 jwir3 commented Aug 17, 2016

Adds two methods: hasQueryParameters() and hasQueryParameters(AbstractMap.SimpleEntry<String, String> params...). The latter version checks whether the request has exactly the query parameters specified, whereas the former checks whether there are any parameters given at all.

@jwir3 jwir3 force-pushed the feature/query-parameters branch 2 times, most recently from b634fd2 to 78723fd Compare August 17, 2016 21:22
};
}

public static RequestMatcher hasQueryParameters(final SimpleEntry<String, String>... expectedParams) {
Copy link
Owner

Choose a reason for hiding this comment

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

I'd preffer here a dedidacted QueryParam class instead of SimpleEntry as so the users won't have problems as of what the parameter is all about.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I can add that.

@jwir3
Copy link
Contributor Author

jwir3 commented Aug 18, 2016

Similar to #40, I don't seem to be able to reproduce the lint errors locally. I can fix them, but I just need to know what's causing them (I can't see the report(s), either on CircleCI).

@jwir3
Copy link
Contributor Author

jwir3 commented Aug 18, 2016

As requested, I added a class called QueryParam. I also changed the method hasQueryParameters(QueryParam... expectedParams) to be named hasExactQueryParameters(QueryParam... expectedParams). I think this makes it more clear that the client needs to match exactly the query parameters given, not just some of them.

@andrzejchm andrzejchm added this to the 0.1.4 milestone Aug 19, 2016
final int idx = pair.indexOf("=");
final String key = idx > 0 ? URLDecoder.decode(pair.substring(0, idx), "UTF-8") : pair;
if (!query_pairs.containsKey(key)) {
query_pairs.put(key, new LinkedList<String>());
Copy link
Owner

Choose a reason for hiding this comment

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

its better to create the list, populate it with items and them put it in a map.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I'll modify this.

@jwir3
Copy link
Contributor Author

jwir3 commented Aug 22, 2016

@andrzejchm I think I addressed your comments in the latest commits.

@jwir3
Copy link
Contributor Author

jwir3 commented Aug 22, 2016

Hm, so CircleCI seems to think something is wrong wrt to the FindBugs configuration, but the findbugs.html report in the artifacts doesn't seem to show anything.

@jwir3
Copy link
Contributor Author

jwir3 commented Aug 22, 2016

Ah. Nevermind. I found the problem. It was complaining about my use of keySet() instead of entrySet().

@jwir3
Copy link
Contributor Author

jwir3 commented Aug 25, 2016

@andrzejchm review ping?

* A key/value set representing query parameters in an HTTP request.
*/
public class QueryParam {
private String key;
Copy link
Owner

Choose a reason for hiding this comment

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

should be final then

Also:
  - Made a small adjustment to the RESTMockServer logger to make it public.
    I also make it, by default, a NoOpLogger, and initialized it
    immediately, rather than initializing it during the constructor (so it
    will never be null).
@jwir3
Copy link
Contributor Author

jwir3 commented Aug 26, 2016

@andrzejchm Added the logging, as requested. Also I modified RESTMockServer.logger to be initialized immediately so that we don't potentially have a race condition where an NPE could crop up (although it's probably super unlikely).

@andrzejchm andrzejchm merged commit 9b2b3be into andrzejchm:develop Aug 29, 2016
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