diff --git a/java/google/registry/batch/BatchModule.java b/java/google/registry/batch/BatchModule.java index 35c915c93..7b34122b6 100644 --- a/java/google/registry/batch/BatchModule.java +++ b/java/google/registry/batch/BatchModule.java @@ -96,6 +96,6 @@ public class BatchModule { @Provides @Parameter(PARAM_RESAVE_TIMES) static ImmutableSet provideResaveTimes(HttpServletRequest req) { - return extractSetOfDatetimeParameters(req, PARAM_RESAVE_TIMES, null); + return extractSetOfDatetimeParameters(req, PARAM_RESAVE_TIMES); } } diff --git a/java/google/registry/module/backend/BackendModule.java b/java/google/registry/module/backend/BackendModule.java index a84a7fa1c..4e09df026 100644 --- a/java/google/registry/module/backend/BackendModule.java +++ b/java/google/registry/module/backend/BackendModule.java @@ -45,8 +45,7 @@ public class BackendModule { @Provides @Parameter(RequestParameters.PARAM_TLDS) static ImmutableSet provideTlds(HttpServletRequest req) { - ImmutableSet tlds = - extractSetOfParameters(req, RequestParameters.PARAM_TLDS, RequestParameters.PARAM_TLD); + ImmutableSet tlds = extractSetOfParameters(req, RequestParameters.PARAM_TLDS); assertTldsExist(tlds); return tlds; } diff --git a/java/google/registry/rde/RdeModule.java b/java/google/registry/rde/RdeModule.java index 403319a79..afe56c2b1 100644 --- a/java/google/registry/rde/RdeModule.java +++ b/java/google/registry/rde/RdeModule.java @@ -59,7 +59,7 @@ public abstract class RdeModule { @Provides @Parameter(PARAM_WATERMARKS) static ImmutableSet provideWatermarks(HttpServletRequest req) { - return extractSetOfDatetimeParameters(req, PARAM_WATERMARKS, PARAM_WATERMARK); + return extractSetOfDatetimeParameters(req, PARAM_WATERMARKS); } @Provides diff --git a/java/google/registry/request/RequestParameters.java b/java/google/registry/request/RequestParameters.java index 23b5b5412..db7cb6c7f 100644 --- a/java/google/registry/request/RequestParameters.java +++ b/java/google/registry/request/RequestParameters.java @@ -23,7 +23,6 @@ import static com.google.common.collect.ImmutableSet.toImmutableSet; import com.google.common.base.Ascii; import com.google.common.base.Splitter; import com.google.common.collect.ImmutableSet; -import com.google.common.flogger.FluentLogger; import google.registry.request.HttpException.BadRequestException; import java.util.Optional; import javax.annotation.Nullable; @@ -33,8 +32,6 @@ import org.joda.time.DateTime; /** Utilities for extracting parameters from HTTP requests. */ public final class RequestParameters { - private static final FluentLogger logger = FluentLogger.forEnclosingClass(); - /** The standardized request parameter name used by any action taking a tld parameter. */ public static final String PARAM_TLD = "tld"; /** The standardized request parameter name used by any action taking multiple tld parameters. */ @@ -101,64 +98,33 @@ public final class RequestParameters { /** * Returns all GET or POST parameters associated with {@code name} (or {@code nameLegacy}). * - *

While transitioning from "param=key1¶m=key2" to "params=key1,key2" style of set inputing - * - we will be accepting all options (with a warning for "wrong" uses). + *

The parameter value is assumed to be a comma-delimited set of values - so tlds=com,net would + * result in ImmutableSet.of("com", "net"). * - *

TODO(b/78226288): remove transition code (including "legacyName") once there are no more - * warnings. + *

Empty strings are not supported, and are automatically removed from the result. + * + *

Both missing parameter and parameter with empty value result in an empty set. * * @param req the request that has the parameter - * @param name the name of the parameter, should be in plural form (e.g. tlds=com,net) - * @param legacyName the legacy, singular form, name. Used only while transitioning from the - * tld=com&tld=net form, in case we forgot to fix some reference. Null if there was no - * singular form to transition from. + * @param name the name of the parameter, should be in plural form (e.g. tlds=, not tld=) */ - public static ImmutableSet extractSetOfParameters( - HttpServletRequest req, String name, @Nullable String legacyName) { + public static ImmutableSet extractSetOfParameters(HttpServletRequest req, String name) { String[] parameters = req.getParameterValues(name); - if (legacyName != null) { - String[] legacyParameters = req.getParameterValues(legacyName); - if (legacyParameters != null) { - if (parameters == null) { - logger.atWarning().log( - "Bad 'set of parameters' input! Used legacy name %s instead of new name %s", - legacyName, name); - parameters = legacyParameters; - } else { - logger.atSevere().log( - "Bad 'set of parameters' input! Used both legacy name %s and new name %s! " - + "Ignoring lagacy name.", - legacyName, name); - } - } - } - if (parameters == null) { + if (parameters == null || parameters.length == 0) { return ImmutableSet.of(); } if (parameters.length > 1) { - logger.atWarning().log( - "Bad 'set of parameters' input! " - + "Received multiple values instead of single comma-delimited value for parameter %s", - name); - return ImmutableSet.copyOf(parameters); + throw new BadRequestException( + String.format( + "Bad 'set of parameters' input! Received multiple values instead of single " + + "comma-delimited value for parameter %s", + name)); } - if (parameters[0].isEmpty()) { - return ImmutableSet.of(); - } - return ImmutableSet.copyOf(Splitter.on(',').split(parameters[0])); - } - - /** - * Returns all GET or POST parameters associated with {@code name}. - * - *

While transitioning from "param=key1¶m=key2" to "params=key1,key2" style of set inputing - * - we will be accepting all options (with a warning for "wrong" uses). - * - *

TODO(b/78226288): remove transition code (including "legacyName") once there are no more - * warnings. - */ - public static ImmutableSet extractSetOfParameters(HttpServletRequest req, String name) { - return extractSetOfParameters(req, name, null); + return Splitter.on(',') + .splitToList(parameters[0]) + .stream() + .filter(s -> !s.isEmpty()) + .collect(toImmutableSet()); } /** @@ -260,9 +226,9 @@ public final class RequestParameters { * @throws BadRequestException if one of the parameter values is not a valid {@link DateTime}. */ public static ImmutableSet extractSetOfDatetimeParameters( - HttpServletRequest req, String name, @Nullable String legacyName) { + HttpServletRequest req, String name) { try { - return extractSetOfParameters(req, name, legacyName) + return extractSetOfParameters(req, name) .stream() .filter(not(String::isEmpty)) .map(DateTime::parse) diff --git a/java/google/registry/tools/server/ToolsServerModule.java b/java/google/registry/tools/server/ToolsServerModule.java index 25762e533..4c42dd668 100644 --- a/java/google/registry/tools/server/ToolsServerModule.java +++ b/java/google/registry/tools/server/ToolsServerModule.java @@ -88,7 +88,7 @@ public class ToolsServerModule { @Provides @Parameter(RequestParameters.PARAM_TLDS) static ImmutableSet provideTlds(HttpServletRequest req) { - return extractSetOfParameters(req, RequestParameters.PARAM_TLDS, RequestParameters.PARAM_TLD); + return extractSetOfParameters(req, RequestParameters.PARAM_TLDS); } @Provides diff --git a/javatests/google/registry/request/RequestParametersTest.java b/javatests/google/registry/request/RequestParametersTest.java index a56f61ab4..0271a5b85 100644 --- a/javatests/google/registry/request/RequestParametersTest.java +++ b/javatests/google/registry/request/RequestParametersTest.java @@ -23,6 +23,7 @@ import static google.registry.request.RequestParameters.extractOptionalEnumParam import static google.registry.request.RequestParameters.extractOptionalParameter; import static google.registry.request.RequestParameters.extractRequiredDatetimeParameter; import static google.registry.request.RequestParameters.extractRequiredParameter; +import static google.registry.request.RequestParameters.extractSetOfParameters; import static google.registry.testing.JUnitBackports.assertThrows; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -78,6 +79,43 @@ public class RequestParametersTest { assertThat(extractOptionalParameter(req, "spin")).isEmpty(); } + @Test + public void testExtractSetOfParameters_notPresent_returnsEmpty() { + assertThat(extractSetOfParameters(req, "spin")).isEmpty(); + } + + @Test + public void testExtractSetOfParameters_empty_returnsEmpty() { + when(req.getParameterValues("spin")).thenReturn(new String[]{""}); + assertThat(extractSetOfParameters(req, "spin")).isEmpty(); + } + + @Test + public void testExtractSetOfParameters_oneValue_returnsValue() { + when(req.getParameterValues("spin")).thenReturn(new String[]{"bog"}); + assertThat(extractSetOfParameters(req, "spin")).containsExactly("bog"); + } + + @Test + public void testExtractSetOfParameters_multipleValues_returnsAll() { + when(req.getParameterValues("spin")).thenReturn(new String[]{"bog,gob"}); + assertThat(extractSetOfParameters(req, "spin")).containsExactly("bog", "gob"); + } + + @Test + public void testExtractSetOfParameters_multipleValuesWithEmpty_removesEmpty() { + when(req.getParameterValues("spin")).thenReturn(new String[]{",bog,,gob,"}); + assertThat(extractSetOfParameters(req, "spin")).containsExactly("bog", "gob"); + } + + @Test + public void testExtractSetOfParameters_multipleParameters_error() { + when(req.getParameterValues("spin")).thenReturn(new String[]{"bog", "gob"}); + BadRequestException thrown = + assertThrows(BadRequestException.class, () -> extractSetOfParameters(req, "spin")); + assertThat(thrown).hasMessageThat().contains("spin"); + } + @Test public void testExtractBooleanParameter_notPresent_returnsFalse() { assertThat(extractBooleanParameter(req, "love")).isFalse();