Skip to content

Commit

Permalink
Add IDL serializer option to coerce inline IO
Browse files Browse the repository at this point in the history
This commit adds a new IDL serializer configuration, coerceInlineIo,
that will use inline IO to write input and output shapes that are only
bound to one operation - even if they don't have the input or output
trait applied.

It also fixes a bug where an empty newline was written before empty
inline IO shape definitions.
  • Loading branch information
kstich committed Jun 11, 2024
1 parent 14a8959 commit 31b4e10
Show file tree
Hide file tree
Showing 4 changed files with 160 additions and 15 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
import java.util.regex.Pattern;
import java.util.stream.Collectors;
import software.amazon.smithy.model.Model;
import software.amazon.smithy.model.knowledge.OperationIndex;
import software.amazon.smithy.model.loader.Prelude;
import software.amazon.smithy.model.node.ArrayNode;
import software.amazon.smithy.model.node.BooleanNode;
Expand Down Expand Up @@ -74,6 +75,7 @@ public final class SmithyIdlModelSerializer {
private final String inlineInputSuffix;
private final String inlineOutputSuffix;
private final boolean inferInlineIoSuffixes;
private final boolean shouldCoerceInlineIo;

/**
* Trait serialization features.
Expand Down Expand Up @@ -114,15 +116,21 @@ private SmithyIdlModelSerializer(Builder builder) {
traitFilter = builder.traitFilter.and(FunctionalUtils.not(Trait::isSynthetic));
basePath = builder.basePath;
if (basePath != null) {
Function<Shape, Path> shapePlacer = builder.shapePlacer;
this.shapePlacer = shape -> this.basePath.resolve(shapePlacer.apply(shape));
Function<Shape, Path> placer = builder.shapePlacer;
shapePlacer = shape -> basePath.resolve(placer.apply(shape));
} else {
this.shapePlacer = builder.shapePlacer;
shapePlacer = builder.shapePlacer;
}
componentOrder = builder.componentOrder;
inlineInputSuffix = builder.inlineInputSuffix;
inlineOutputSuffix = builder.inlineOutputSuffix;
shouldCoerceInlineIo = builder.shouldCoerceInlineIo;
// Force inferring suffixes on if coercion is on.
if (shouldCoerceInlineIo) {
inferInlineIoSuffixes = true;
} else {
inferInlineIoSuffixes = builder.inferInlineIoSuffixes;
}
this.componentOrder = builder.componentOrder;
this.inlineInputSuffix = builder.inlineInputSuffix;
this.inlineOutputSuffix = builder.inlineOutputSuffix;
this.inferInlineIoSuffixes = builder.inferInlineIoSuffixes;
}

/**
Expand Down Expand Up @@ -193,6 +201,7 @@ private Set<ShapeId> getInlineableShapes(
String inputSuffix,
String outputSuffix
) {
OperationIndex operationIndex = OperationIndex.of(fullModel);
Set<ShapeId> inlineableShapes = new HashSet<>();
for (Shape shape : shapes) {
if (!shape.isOperationShape()) {
Expand All @@ -201,14 +210,14 @@ private Set<ShapeId> getInlineableShapes(
OperationShape operation = shape.asOperationShape().get();
if (!operation.getInputShape().equals(UnitTypeTrait.UNIT)) {
Shape inputShape = fullModel.expectShape(operation.getInputShape());
if (shapes.contains(inputShape) && inputShape.hasTrait(InputTrait.ID)
if (shapes.contains(inputShape) && shouldInlineInputShape(operationIndex, inputShape)
&& inputShape.getId().getName().equals(operation.getId().getName() + inputSuffix)) {
inlineableShapes.add(operation.getInputShape());
}
}
if (!operation.getOutputShape().equals(UnitTypeTrait.UNIT)) {
Shape outputShape = fullModel.expectShape(operation.getOutputShape());
if (shapes.contains(outputShape) && outputShape.hasTrait(OutputTrait.ID)
if (shapes.contains(outputShape) && shouldInlineOutputShape(operationIndex, outputShape)
&& outputShape.getId().getName().equals(operation.getId().getName() + outputSuffix)) {
inlineableShapes.add(operation.getOutputShape());
}
Expand All @@ -217,6 +226,18 @@ private Set<ShapeId> getInlineableShapes(
return inlineableShapes;
}

private boolean shouldInlineInputShape(OperationIndex operationIndex, Shape target) {
// Only allow coercion if the shape is only used as one input.
return target.hasTrait(InputTrait.ID)
|| (shouldCoerceInlineIo && operationIndex.getInputBindings(target).size() == 1);
}

private boolean shouldInlineOutputShape(OperationIndex operationIndex, Shape target) {
// Only allow coercion if the shape is only used as one output.
return target.hasTrait(OutputTrait.ID)
|| (shouldCoerceInlineIo && operationIndex.getOutputBindings(target).size() == 1);
}

private Pair<String, String> determineInlineSuffixes(Model fullModel, Collection<Shape> shapes) {
if (!inferInlineIoSuffixes) {
return Pair.of(inlineInputSuffix, inlineOutputSuffix);
Expand Down Expand Up @@ -330,6 +351,7 @@ public static final class Builder implements SmithyBuilder<SmithyIdlModelSeriali
private String inlineInputSuffix = DEFAULT_INLINE_INPUT_SUFFIX;
private String inlineOutputSuffix = DEFAULT_INLINE_OUTPUT_SUFFIX;
private boolean inferInlineIoSuffixes = false;
private boolean shouldCoerceInlineIo = false;

public Builder() {}

Expand Down Expand Up @@ -457,11 +479,30 @@ public Builder inlineOutputSuffix(String inlineOutputSuffix) {
* <p>The suffixes set by {@link #inlineInputSuffix(String)} and {@link #inlineOutputSuffix(String)}
* will be the default values.
*
* @param shouldinferInlineIoSuffixes Whether inline IO suffixes should be inferred for each file.
* @param shouldInferInlineIoSuffixes Whether inline IO suffixes should be inferred for each file.
* @return Returns the builder.
*/
public Builder inferInlineIoSuffixes(boolean shouldInferInlineIoSuffixes) {
this.inferInlineIoSuffixes = shouldInferInlineIoSuffixes;
return this;
}

/**
* Determines whether inline IO should be coerced for shapes operation input
* and output that does not have the {@code @input} or {@code @output} trait,
* respectively.
*
* <p>If true, this will determine any shared IO suffixes for each file. Only the
* shapes present within each file will impact what that file's suffixes will be.
*
* <p>The suffixes set by {@link #inlineInputSuffix(String)} and {@link #inlineOutputSuffix(String)}
* will be the default values.
*
* @param shouldCoerceInlineIo Whether inline IO should be coerced for each file.
* @return Returns the builder.
*/
public Builder inferInlineIoSuffixes(boolean shouldinferInlineIoSuffixes) {
this.inferInlineIoSuffixes = shouldinferInlineIoSuffixes;
public Builder coerceInlineIo(boolean shouldCoerceInlineIo) {
this.shouldCoerceInlineIo = shouldCoerceInlineIo;
return this;
}

Expand Down Expand Up @@ -833,7 +874,7 @@ private Collection<MemberShape> writeInlineableProperty(String key, ShapeId shap
}

private boolean hasOnlyDefaultTrait(Shape shape, ShapeId defaultTrait) {
return shape.getAllTraits().size() == 1 && shape.hasTrait(defaultTrait);
return shape.getAllTraits().isEmpty() || (shape.getAllTraits().size() == 1 && shape.hasTrait(defaultTrait));
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import static org.hamcrest.Matchers.hasKey;
import static org.hamcrest.Matchers.is;
import static org.hamcrest.Matchers.not;
import static org.junit.jupiter.api.Assertions.assertEquals;

import java.io.IOException;
import java.net.URISyntaxException;
Expand Down Expand Up @@ -49,7 +50,7 @@ public void testConversion(Path path) {
}

String serializedString = serialized.entrySet().iterator().next().getValue();
Assertions.assertEquals(IoUtils.readUtf8File(path).replaceAll("\\R", "\n"), serializedString);
assertEquals(IoUtils.readUtf8File(path).replaceAll("\\R", "\n"), serializedString);
}

@Test
Expand Down Expand Up @@ -259,7 +260,7 @@ public void handlesEnumMixins() {
String expectedOutput = IoUtils.readUtf8Resource(getClass(), "idl-serialization/enum-mixin-output.smithy")
.replaceAll("\\R", "\n");
String serializedString = serialized.entrySet().iterator().next().getValue();
Assertions.assertEquals(expectedOutput, serializedString);
assertEquals(expectedOutput, serializedString);
}

@Test
Expand Down Expand Up @@ -306,4 +307,21 @@ public void canInferInlineSuffixes() {
assertThat(actual, equalTo(expected));
}
}

@Test
public void coercesInlineIO() {
Model before = Model.assembler()
.addImport(getClass().getResource("idl-serialization/coerced-io/before.smithy"))
.assemble().unwrap();

Map<Path, String> reserialized = SmithyIdlModelSerializer.builder()
.coerceInlineIo(true)
.build()
.serialize(before);
String modelResult = reserialized.values().iterator().next().replace("\r\n", "\n");

String expected = IoUtils.readUtf8Url(getClass().getResource("idl-serialization/coerced-io/after.smithy"))
.replace("\r\n", "\n");
assertEquals(expected, modelResult);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
$version: "2.0"
$operationInputSuffix: "Request"
$operationOutputSuffix: "Response"

namespace com.example

service InlineService {
operations: [
MultipleBind1
MultipleBind2
WithoutTraits
WithTraits
]
}

operation MultipleBind1 {
input: MultipleBindRequest
output: MultipleBindResponse
}

operation MultipleBind2 {
input: MultipleBindRequest
output: MultipleBindResponse
}

operation WithoutTraits {
input := {}
output := {}
}

operation WithTraits {
input := {
a: String
}
output := {}
}

structure MultipleBindRequest {}

structure MultipleBindResponse {}
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
$version: "2.0"

namespace com.example

service InlineService {
operations: [
MultipleBind1
MultipleBind2
WithoutTraits
WithTraits
]
}

operation MultipleBind1 {
input: MultipleBindRequest
output: MultipleBindResponse
}

operation MultipleBind2 {
input: MultipleBindRequest
output: MultipleBindResponse
}

operation WithoutTraits {
input: WithoutTraitsRequest
output: WithoutTraitsResponse
}

operation WithTraits {
input: WithTraitsRequest
output: WithTraitsResponse
}

structure MultipleBindRequest {}

structure MultipleBindResponse {}

structure WithoutTraitsRequest {}

structure WithoutTraitsResponse {}

structure WithTraitsRequest {
a: String
}

structure WithTraitsResponse {}

0 comments on commit 31b4e10

Please sign in to comment.