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.

Sign up for GitHub

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 new methods to manage the path params map #2645

Open
wants to merge 2 commits into
base: 4.x
Choose a base branch
from
Open

Adding new methods to manage the path params map #2645

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
28d5a15
Adding new methods to manage the path params map
franz1981 May 6, 2024
0d6cdeb
Update vertx-web/src/main/java/io/vertx/ext/web/RoutingContext.java
franz1981 Oct 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions ...pi-contract/src/main/java/io/vertx/ext/web/api/validation/impl/BaseValidationHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,12 +101,13 @@ public void handle(RoutingContext routingContext) {
private Map<String, RequestParameter> validatePathParams(RoutingContext routingContext) throws ValidationException {
// Validation process validate only params that are registered in the validation -> extra params are allowed
Map<String, RequestParameter> parsedParams = new HashMap<>();
Map<String, String> pathParams = routingContext.pathParams();

for (ParameterValidationRule rule : pathParamsRules.values()) {
String name = rule.getName();
if (pathParams.containsKey(name)) {
if (pathParams.get(name) != null || !rule.isOptional() ) {
RequestParameter parsedParam = rule.validateSingleParam(pathParams.get(name));
if (name != null) {
String pathParam = routingContext.pathParam(name);
if (pathParam != null || !rule.isOptional() ) {
RequestParameter parsedParam = rule.validateSingleParam(pathParam);
if (parsedParams.containsKey(parsedParam.getName()))
parsedParam = parsedParam.merge(parsedParams.get(parsedParam.getName()));
parsedParams.put(parsedParam.getName(), parsedParam);
Expand Down
15 changes: 14 additions & 1 deletion vertx-web/src/main/java/io/vertx/ext/web/RoutingContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -613,7 +613,20 @@ default LanguageHeader preferredLanguage() {
}

/**
* Returns a map of named parameters as defined in path declaration with their actual values
* Add a new one or replace an existing path parameter
* @throws NullPointerException when the name or value is null
*/
void addOrReplacePathParam(String name, String value);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really need this new method? It will create the underlying map if needed anyway.

Copy link
Author

Choose a reason for hiding this comment

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

good point; it was consistent to not expose the underlying form/nature/content of the map - in order to enable to not allocate it, if possible

But any suggestion is more than welcome, bud 🙏 and thanks for looking at this!


/**
* Remove a path parameter.
*
* @return {@code true} when removed, {@code false} otherwise
*/
boolean removePathParam(String s);

/**
* Returns an unmodifiable map of named parameters as defined in path declaration with their actual values
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a breaking change that is not mandatory (even if I understand the intention). Can you please revert it?

Copy link
Author

Choose a reason for hiding this comment

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

Do you mean that you prefer to expose it directly as it was? without being not modifiable?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, because we some users will expect this to be modifiable.

*
* @return the map of named parameters
*/
Expand Down
18 changes: 7 additions & 11 deletions vertx-web/src/main/java/io/vertx/ext/web/impl/RouteState.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1001,8 +1001,7 @@ public int matches(RoutingContextImplBase context, String mountPoint, boolean fa
}
if (pattern != null) {
// need to reset "rest"
context.pathParams()
.remove("*");
context.removePathParam("*");

String path = useNormalizedPath ? context.normalizedPath() : context.request().path();

Expand Down Expand Up @@ -1031,8 +1030,7 @@ public int matches(RoutingContextImplBase context, String mountPoint, boolean fa
if (!exactPath) {
context.matchRest = m.start("rest");
// always replace
context.pathParams()
.put("*", path.substring(context.matchRest));
context.addOrReplacePathParam("*", path.substring(context.matchRest));
}

if (!isEmpty(groups)) {
Expand Down Expand Up @@ -1163,8 +1161,7 @@ private boolean pathMatches(String mountPoint, RoutingContext ctx) {

if (exactPath) {
// exact path has no "rest"
ctx.pathParams()
.remove("*");
ctx.removePathParam("*");

return pathMatchesExact(thePath, requestPath, pathEndsWithSlash);
} else {
Expand All @@ -1185,17 +1182,16 @@ private boolean pathMatches(String mountPoint, RoutingContext ctx) {
// because the mount path ended with a wildcard we are relaxed in the check
if (thePath.regionMatches(0, requestPath, 0, pathLen - 1)) {
// handle the "rest" as path param *, always known to be empty
ctx.pathParams()
.put("*", "/");
ctx.addOrReplacePathParam("*", "/");
return true;
}
}
}

if (requestPath.startsWith(thePath)) {
// handle the "rest" as path param *
ctx.pathParams()
.put("*", URIDecoder.decodeURIComponent(requestPath.substring(thePath.length()), false));
ctx.addOrReplacePathParam("*",
URIDecoder.decodeURIComponent(requestPath.substring(thePath.length()), false));
return true;
}
return false;
Expand Down Expand Up @@ -1268,7 +1264,7 @@ private void addPathParam(RoutingContext context, String name, String value) {
if (!request.params().contains(name)) {
request.params().add(name, decodedValue);
}
context.pathParams().put(name, decodedValue);
context.addOrReplacePathParam(name, decodedValue);
}

boolean hasNextContextHandler(RoutingContextImplBase context) {
Expand Down
10 changes: 10 additions & 0 deletions vertx-web/src/main/java/io/vertx/ext/web/impl/RoutingContextDecorator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,16 @@ public void reroute(HttpMethod method, String path) {
decoratedContext.reroute(method, path);
}

@Override
public void addOrReplacePathParam(String name, String value) {
decoratedContext.addOrReplacePathParam(name, value);
}

@Override
public boolean removePathParam(String s) {
return decoratedContext.removePathParam(s);
}

@Override
public Map<String, String> pathParams() {
return decoratedContext.pathParams();
Expand Down
37 changes: 33 additions & 4 deletions vertx-web/src/main/java/io/vertx/ext/web/impl/RoutingContextImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
import java.nio.charset.Charset;
import java.util.*;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.function.BiConsumer;
import java.util.stream.Collectors;

import static io.vertx.ext.web.handler.impl.SessionHandlerImpl.SESSION_USER_HOLDER_KEY;
Expand Down Expand Up @@ -439,14 +440,41 @@ public void reroute(HttpMethod method, String path) {
restart();
}

@Override
public void addOrReplacePathParam(final String name, final String value) {
Objects.requireNonNull(name, "name");
Objects.requireNonNull(value, "value");
getOrCreatePathParams().put(name, value);
}

@Override
public Map<String, String> pathParams() {
return getPathParams();
if (pathParams == null || pathParams.isEmpty()) {
return Collections.emptyMap();
}
return Collections.unmodifiableMap(pathParams);
}

@Override
public boolean removePathParam(String s) {
if (s == null) {
return false;
}
if (pathParams != null) {
return pathParams.remove(s) != null;
}
return false;
}

@Override
public @Nullable String pathParam(String name) {
return getPathParams().get(name);
if (name == null) {
return null;
}
if (pathParams != null) {
return pathParams.get(name);
}
return null;
}

@Override
Expand Down Expand Up @@ -490,9 +518,10 @@ private MultiMap getQueryParams(Charset charset) {
return queryParams;
}

private Map<String, String> getPathParams() {
private Map<String, String> getOrCreatePathParams() {
if (pathParams == null) {
pathParams = new HashMap<>();
// let's start small
pathParams = new HashMap<>(1);
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this too aggressive? How about using 4, which allows to store 3 path params without rehashing?

Copy link
Author

Choose a reason for hiding this comment

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

in theory with 1 it should have 2 as capacity - but yes, anything but the default can be a good improvement; given that it happen in our hot path, saving half array is still wellcome!

Copy link
Contributor

Choose a reason for hiding this comment

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

Then, let's be a little conservative and go with 4.

}
return pathParams;
}
Expand Down
11 changes: 11 additions & 0 deletions vertx-web/src/main/java/io/vertx/ext/web/impl/RoutingContextWrapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.function.BiConsumer;

/**
* @author <a href="http://tfox.org">Tim Fox</a>
Expand Down Expand Up @@ -308,6 +309,16 @@ public void reroute(HttpMethod method, String path) {
inner.reroute(method, path);
}

@Override
public void addOrReplacePathParam(final String name, final String value) {
inner.addOrReplacePathParam(name, value);
}

@Override
public boolean removePathParam(final String s) {
return inner.removePathParam(s);
}

@Override
public Map<String, String> pathParams() {
return inner.pathParams();
Expand Down
6 changes: 2 additions & 4 deletions vertx-web/src/test/java/io/vertx/ext/web/RouterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -863,8 +863,7 @@ public void testPercentEncoding() throws Exception {
@Test
public void testPathParamsAreFulfilled() throws Exception {
router.route("/blah/:abc/quux/:def/eep/:ghi").handler(rc -> {
Map<String, String> params = rc.pathParams();
rc.response().setStatusMessage(params.get("abc") + params.get("def") + params.get("ghi")).end();
rc.response().setStatusMessage(rc.pathParam("abc") + rc.pathParam("def") + rc.pathParam("ghi")).end();
});
testPattern("/blah/tim/quux/julien/eep/nick", "timjuliennick");
}
Expand All @@ -877,11 +876,10 @@ public void testPathParamsDoesNotOverrideQueryParam() throws Exception {
final String queryParamValue2 = "queryParamValue2";
final String sep = ",";
router.route("/blah/:" + paramName + "/test").handler(rc -> {
Map<String, String> params = rc.pathParams();
MultiMap queryParams = rc.request().params();
List<String> values = queryParams.getAll(paramName);
String qValue = values.stream().collect(Collectors.joining(sep));
rc.response().setStatusMessage(params.get(paramName) + "|" + qValue).end();
rc.response().setStatusMessage(rc.pathParam(paramName) + "|" + qValue).end();
});
testRequest(HttpMethod.GET,
"/blah/" + pathParamValue + "/test?" + paramName + "=" + queryParamValue1 + "&" + paramName + "=" + queryParamValue2,
Expand Down
Loading