NEXUS-10527 Added validation support to httpfixture. #49
Conversation
| @@ -28,7 +28,7 @@ | |||
| <artifactId>goodies</artifactId> | |||
| <name>${project.groupId}:${project.artifactId}</name> | |||
| <packaging>pom</packaging> | |||
| <version>2.1.3-SNAPSHOT</version> | |||
| <version>2.2.0-SNAPSHOT</version> | |||
There was a problem hiding this comment.
Note, updated to 2.2.0 to represent added functionality. Let me know if this version doesn't seem appropriate for some reason.
|
This seems to conflate two things which should really have been two PRs:
I can see the value of the real proxy but I'm currently not convinced that this |
|
@bentmann The behavior implmentation was moved to NX3, see #911. |
| /** | ||
| * Reset the success counter to zero. | ||
| */ | ||
| public void resetSuccessCount(); |
There was a problem hiding this comment.
With the recent refactoring HttpValidator implementations should be cheap to create, so there's no reason to have static instances of them. This means there's less reason to have a resetSuccessCount method, in fact it would be better to remove this just to make sure it can't be accidentally reset.
Note that other 'counting' behaviours in this codebase (BasicAuth and Fail) don't include reset methods - instead they expect each test to use a distinct instance, as that means the tests can be run in parallel.
There was a problem hiding this comment.
After thinking this over it's much cleaner to track success/failures in the validating behaviour according to the result of the validate method as suggested above, in which case both counting methods can be removed from this API.
There was a problem hiding this comment.
As mentioned in the NX3 PR, moving the count/reset out one layer to the behavior or proxy server with validation.
|
+1 |
| @@ -20,7 +20,7 @@ | |||
| <parent> | |||
| <groupId>org.sonatype.goodies</groupId> | |||
| <artifactId>goodies</artifactId> | |||
| <version>2.1.3-SNAPSHOT</version> | |||
| <version>2.2.0-SNAPSHOT</version> | |||
There was a problem hiding this comment.
has there already been a discussion of 2.1.3 VS 2.2.0?
There was a problem hiding this comment.
No one has mentioned anything about the version. I switched everything to 2.2.0 because it seems like adding new features warrants a new minor version at least.
|
Needs a Bamboo build please :) |
|
|
||
| @Override | ||
| public AsyncContext startAsync(ServletRequest servletRequest, ServletResponse servletResponse) | ||
| throws IllegalStateException |
There was a problem hiding this comment.
Comes from the HttpServeltRequest interface, nothing to do here.
| @Override | ||
| public HttpResponse clientToProxyRequest(HttpObject httpObject) { | ||
| for (HttpValidator v : validation) { | ||
| v.validate(new NettyHttpRequestWrapper(originalRequest)); |
There was a problem hiding this comment.
Move this wrapping outside of this loop to only incur the cost once, rather than for every validation.
mcculls
changed the title
| private AtomicInteger successCount = new AtomicInteger(); | ||
|
|
||
| public ValidatingBehavior(HttpValidator... validators) { | ||
| checkArgument(validators != null && validators.length > 0, "Must have at least one validator set."); |
There was a problem hiding this comment.
Minor: ValidatingBehavior(null,null) could sneak in some nulls and NPE down on line 53.
There was a problem hiding this comment.
Adding more robust verification of validators.
|
+1 |
1 similar comment
|
+1 |
|
That last commit msg is incorrect, i fixed the "withPort" method so it takes effect. |
…refactored to ValidatingProxyServer.
|
+1 |
Jira: https://issues.sonatype.org/browse/NEXUS-10527
Bamboo: http://bamboo.s/browse/OSS-GOODIESF15-9
Added validating proxy server and validation support to goodies-httpfixture.