diff --git a/CHANGELOG.md b/CHANGELOG.md index 2139b2b2d..4fcf6dfac 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,10 @@ `NullPointerException` from `body()`, `charset()`, `isBinary()` and `length()`. A request with no body reports a `null` body and charset, is binary, and has length 0. Its `toString()` prints the request line and headers without a body (#1210). +* Form encoders can process primitive array fields without failing on an `Object[]` cast. + URL-encoded fields use the request's `CollectionFormat`. In multipart requests, numeric and + boolean arrays create repeated parts, and `byte[]` values stay as one binary part. Multipart + `char[]` values are still passed to the delegate encoder (#3607). ### Version 13.16 diff --git a/form/src/main/java/feign/form/UrlencodedFormContentProcessor.java b/form/src/main/java/feign/form/UrlencodedFormContentProcessor.java index 6c82a5c9d..db9517bd6 100644 --- a/form/src/main/java/feign/form/UrlencodedFormContentProcessor.java +++ b/form/src/main/java/feign/form/UrlencodedFormContentProcessor.java @@ -16,13 +16,13 @@ package feign.form; import static feign.form.ContentType.URLENCODED; +import static feign.form.util.Elements.elementsOf; import feign.CollectionFormat; import feign.RequestTemplate; import feign.codec.EncodeException; import java.net.URLEncoder; import java.nio.charset.Charset; -import java.util.Arrays; import java.util.Collection; import java.util.Collections; import java.util.Map; @@ -30,6 +30,7 @@ import java.util.Objects; import java.util.stream.Collectors; import java.util.stream.Stream; +import java.util.stream.StreamSupport; import lombok.SneakyThrows; import lombok.val; @@ -89,12 +90,12 @@ private CharSequence createKeyValuePair( if (value == null) { return encodedKey; - } else if (value.getClass().isArray()) { + } else if (value.getClass().isArray() || value instanceof Collection) { return createKeyValuePair( - collectionFormat, encodedKey, Arrays.stream((Object[]) value), charset); - } else if (value instanceof Collection) { - return createKeyValuePair( - collectionFormat, encodedKey, ((Collection) value).stream(), charset); + collectionFormat, + encodedKey, + StreamSupport.stream(elementsOf(value).spliterator(), false), + charset); } return new StringBuilder() .append(encodedKey) diff --git a/form/src/main/java/feign/form/multipart/ManyParametersWriter.java b/form/src/main/java/feign/form/multipart/ManyParametersWriter.java index 6e743ea21..4238ad813 100644 --- a/form/src/main/java/feign/form/multipart/ManyParametersWriter.java +++ b/form/src/main/java/feign/form/multipart/ManyParametersWriter.java @@ -15,6 +15,7 @@ */ package feign.form.multipart; +import static feign.form.util.Elements.elementsOf; import static lombok.AccessLevel.PRIVATE; import feign.codec.EncodeException; @@ -33,31 +34,15 @@ public class ManyParametersWriter extends AbstractWriter { @Override public boolean isApplicable(Object value) { - if (value.getClass().isArray()) { - Object[] values = (Object[]) value; - return values.length > 0 && parameterWriter.isApplicable(values[0]); - } - if (!(value instanceof Iterable)) { - return false; - } - val iterable = (Iterable) value; - val iterator = iterable.iterator(); + val iterator = elementsOf(value).iterator(); return iterator.hasNext() && parameterWriter.isApplicable(iterator.next()); } @Override public void write(Output output, String boundary, String key, Object value) throws EncodeException { - if (value.getClass().isArray()) { - val objects = (Object[]) value; - for (val object : objects) { - parameterWriter.write(output, boundary, key, object); - } - } else if (value instanceof Iterable) { - val iterable = (Iterable) value; - for (val object : iterable) { - parameterWriter.write(output, boundary, key, object); - } + for (val object : elementsOf(value)) { + parameterWriter.write(output, boundary, key, object); } } } diff --git a/form/src/main/java/feign/form/util/Elements.java b/form/src/main/java/feign/form/util/Elements.java new file mode 100644 index 000000000..1e6fe5576 --- /dev/null +++ b/form/src/main/java/feign/form/util/Elements.java @@ -0,0 +1,41 @@ +/* + * Copyright © 2012 The Feign Authors (feign@commonhaus.dev) + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package feign.form.util; + +import java.lang.reflect.Array; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; + +public final class Elements { + + private Elements() {} + + public static Iterable elementsOf(Object value) { + if (value instanceof Iterable) { + return (Iterable) value; + } + if (value == null || !value.getClass().isArray()) { + return Collections.emptyList(); + } + int length = Array.getLength(value); + List elements = new ArrayList<>(length); + for (int index = 0; index < length; index++) { + elements.add(Array.get(value, index)); + } + return elements; + } +} diff --git a/form/src/test/java/feign/form/UrlencodedFormContentProcessorTest.java b/form/src/test/java/feign/form/UrlencodedFormContentProcessorTest.java index be915fab9..81327533a 100644 --- a/form/src/test/java/feign/form/UrlencodedFormContentProcessorTest.java +++ b/form/src/test/java/feign/form/UrlencodedFormContentProcessorTest.java @@ -28,7 +28,11 @@ import java.util.LinkedHashMap; import java.util.Map; import java.util.function.BiFunction; +import java.util.stream.Stream; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; class UrlencodedFormContentProcessorTest { @@ -116,6 +120,36 @@ void collectionValueUsesPipesCollectionFormat() { Arrays.asList("one", "two"), Client::mapPipes); } + static Stream primitiveArrays() { + return Stream.of( + Arguments.of(new boolean[] {true, false}, "tags=true&tags=false"), + Arguments.of(new byte[] {1, 2}, "tags=1&tags=2"), + Arguments.of(new char[] {'a', 'b'}, "tags=a&tags=b"), + Arguments.of(new double[] {1.25, 2.5}, "tags=1.25&tags=2.5"), + Arguments.of(new float[] {1.5F, 2.5F}, "tags=1.5&tags=2.5"), + Arguments.of(new int[] {1, 2}, "tags=1&tags=2"), + Arguments.of(new long[] {10L, 20L}, "tags=10&tags=20"), + Arguments.of(new short[] {3, 4}, "tags=3&tags=4")); + } + + @ParameterizedTest + @MethodSource("primitiveArrays") + void primitiveArrayValueUsesDefaultExplodedCollectionFormat( + Object tags, String expectedTagFields) { + assertEncodedBody("from=%2B987654321&to=%2B123456789&" + expectedTagFields, tags, Client::map); + } + + @Test + void primitiveArrayIsJoinedIntoOneFieldWithCsvCollectionFormat() { + assertEncodedBody( + "from=%2B987654321&to=%2B123456789&tags=1%2C2", new int[] {1, 2}, Client::mapCsv); + } + + @Test + void emptyPrimitiveArrayValueAddsNoTagsField() { + assertEncodedBody("from=%2B987654321&to=%2B123456789&", new int[0], Client::map); + } + private void assertEncodedBody( String expectedBody, Object tags, diff --git a/form/src/test/java/feign/form/multipart/ManyParametersWriterTest.java b/form/src/test/java/feign/form/multipart/ManyParametersWriterTest.java new file mode 100644 index 000000000..21f1a61c0 --- /dev/null +++ b/form/src/test/java/feign/form/multipart/ManyParametersWriterTest.java @@ -0,0 +1,140 @@ +/* + * Copyright © 2012 The Feign Authors (feign@commonhaus.dev) + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package feign.form.multipart; + +import static java.nio.charset.StandardCharsets.ISO_8859_1; +import static java.nio.charset.StandardCharsets.UTF_8; +import static org.assertj.core.api.Assertions.assertThat; + +import feign.RequestTemplate; +import feign.form.FormEncoder; +import java.io.File; +import java.util.List; +import java.util.Map; +import java.util.regex.Pattern; +import org.junit.jupiter.api.Test; + +class ManyParametersWriterTest { + + private static final ManyParametersWriter WRITER = new ManyParametersWriter(); + + private static final String FIXED_BOUNDARY = "boundary"; + + private static final String TEXT_PLAIN_CONTENT_TYPE_HEADER = + "Content-Type: text/plain; charset=UTF-8\r\n\r\n"; + + @Test + void intBooleanAndStringArraysAreApplicable() { + assertThat(WRITER.isApplicable(new int[] {1, 2})).isTrue(); + assertThat(WRITER.isApplicable(new boolean[] {true, false})).isTrue(); + assertThat(WRITER.isApplicable(new String[] {"one", "two"})).isTrue(); + } + + @Test + void byteArrayIsApplicableBecauseByteIsANumber() { + assertThat(WRITER.isApplicable(new byte[] {1, 2})).isTrue(); + } + + @Test + void emptyArrayIsNotApplicable() { + assertThat(WRITER.isApplicable(new int[0])).isFalse(); + assertThat(WRITER.isApplicable(new String[0])).isFalse(); + } + + @Test + void iterableOfParametersIsApplicable() { + assertThat(WRITER.isApplicable(List.of(1, 2))).isTrue(); + assertThat(WRITER.isApplicable(List.of("one", "two"))).isTrue(); + } + + @Test + void emptyIterableIsNotApplicable() { + assertThat(WRITER.isApplicable(List.of())).isFalse(); + } + + @Test + void scalarAndIterableOfFilesAreNotApplicable() { + assertThat(WRITER.isApplicable("one")).isFalse(); + assertThat(WRITER.isApplicable(List.of(new File("file.txt")))).isFalse(); + } + + @Test + void iterableIsWrittenAsOnePartPerElement() { + Output output = new Output(UTF_8); + + WRITER.write(output, FIXED_BOUNDARY, "tags", List.of("one", "two")); + + assertThat(new String(output.toByteArray(), UTF_8)) + .isEqualTo(formatDelimitedTextPart("tags", "one") + formatDelimitedTextPart("tags", "two")); + } + + @Test + void intAndLongArraysAreWrittenAsOneTextPartPerElement() { + Output output = new Output(UTF_8); + + WRITER.write(output, FIXED_BOUNDARY, "ids", new int[] {1, 2}); + WRITER.write(output, FIXED_BOUNDARY, "times", new long[] {10L, 20L}); + + assertThat(new String(output.toByteArray(), UTF_8)) + .isEqualTo( + formatDelimitedTextPart("ids", "1") + + formatDelimitedTextPart("ids", "2") + + formatDelimitedTextPart("times", "10") + + formatDelimitedTextPart("times", "20")); + } + + @Test + void primitiveBooleanArrayIsWrittenAsOnePartPerElement() { + Output output = new Output(UTF_8); + + WRITER.write(output, FIXED_BOUNDARY, "flags", new boolean[] {true, false}); + + assertThat(new String(output.toByteArray(), UTF_8)) + .isEqualTo( + formatDelimitedTextPart("flags", "true") + formatDelimitedTextPart("flags", "false")); + } + + @Test + void byteArrayIsWrittenAsOneBinaryPartInsteadOfRepeatedParts() { + String body = + new String( + encodeMultipart(Map.of("blob", new byte[] {0, 1, 42})), ISO_8859_1); + + assertThat(Pattern.compile("name=\"blob\"").matcher(body).results()).hasSize(1); + assertThat(body).contains("Content-Type: application/octet-stream"); + } + + private static String formatDelimitedTextPart(String name, String payload) { + return "--" + + FIXED_BOUNDARY + + "\r\n" + + "Content-Disposition: form-data; name=\"" + + name + + "\"\r\n" + + TEXT_PLAIN_CONTENT_TYPE_HEADER + + payload + + "\r\n"; + } + + private static byte[] encodeMultipart(Map formFields) { + RequestTemplate template = new RequestTemplate(); + template.header("Content-Type", "multipart/form-data"); + + new FormEncoder().encode(formFields, Map.class, template); + + return template.body(); + } +}