Skip to content

Commit

Permalink
Warn when using jsonAdd with schemas
Browse files Browse the repository at this point in the history
Using the jsonAdd feature with the Smithy to OpenAPI converter is
probably never what you actually want to do. It means that clients,
servers, and other artifacts generated from the Smithy model don't know
about the shapes being defined only in OpenAPI. This PR adds a severe
log warning when this is detected.
  • Loading branch information
mtdowling committed Jun 30, 2021
1 parent 31c66f2 commit 8634843
Show file tree
Hide file tree
Showing 2 changed files with 63 additions and 4 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,14 @@ public ObjectNode updateNode(Context<? extends Trait> context, OpenApi openapi,
for (Map.Entry<String, Node> entry : add.entrySet()) {
try {
LOGGER.info(() -> "OpenAPI `jsonAdd`: adding `" + entry.getKey() + "`");

if (entry.getKey().startsWith("/components/schemas")) {
LOGGER.severe("Adding schemas to the generated OpenAPI model directly means that "
+ "clients, servers, and other artifacts generated from your Smithy "
+ "model don't know about all of the shapes used in the service. You "
+ "almost certainly should not do this.");
}

result = NodePointer.parse(entry.getKey())
.addWithIntermediateValues(result, entry.getValue().toNode())
.expectObjectNode();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,11 @@

package software.amazon.smithy.openapi.fromsmithy.mappers;

import java.util.logging.Handler;
import java.util.logging.LogRecord;
import java.util.logging.Logger;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.Test;
import software.amazon.smithy.model.Model;
import software.amazon.smithy.model.node.Node;
Expand All @@ -26,16 +30,22 @@
import software.amazon.smithy.openapi.fromsmithy.OpenApiConverter;

public class OpenApiJsonAddTest {
@Test
public void addsWithPointers() {
Model model = Model.assembler()

private static Model MODEL;

@BeforeAll
public static void before() {
MODEL = Model.assembler()
// Reusing another test cases's model, but that doesn't matter for the
// purpose of this test.
.addImport(RemoveUnusedComponentsTest.class.getResource("substitutions.smithy"))
.discoverModels()
.assemble()
.unwrap();
}

@Test
public void addsWithPointers() {
ObjectNode addNode = Node.objectNodeBuilder()
.withMember("/info/description", "hello")
.withMember("/info/foo", "bar")
Expand All @@ -49,7 +59,7 @@ public void addsWithPointers() {

ObjectNode openApi = OpenApiConverter.create()
.config(config)
.convertToNode(model);
.convertToNode(MODEL);

String description = NodePointer.parse("/info/description").getValue(openApi).expectStringNode().getValue();
String infoFoo = NodePointer.parse("/info/foo").getValue(openApi).expectStringNode().getValue();
Expand All @@ -61,4 +71,45 @@ public void addsWithPointers() {
Assertions.assertEquals("nested", infoNested);
Assertions.assertEquals("custom", infoTitle);
}

private static final class SearchingHandler extends Handler {
boolean found;
String searchString;

SearchingHandler(String searchString) {
this.searchString = searchString;
}

@Override
public void publish(LogRecord record) {
if (record.getMessage().contains(searchString)) {
found = true;
}
}

@Override
public void flush() {}

@Override
public void close() throws SecurityException {}
}

@Test
public void warnsWhenAddingSchemas() {
Logger logger = Logger.getLogger(OpenApiJsonAdd.class.getName());
SearchingHandler handler = new SearchingHandler("Adding schemas to the generated OpenAPI model directly");
logger.addHandler(handler);

ObjectNode addNode = Node.objectNode()
.withMember("/components/schemas/Merged", Node.objectNode().withMember("type", "string"));

OpenApiConfig config = new OpenApiConfig();
config.setService(ShapeId.from("smithy.example#Service"));
config.setJsonAdd(addNode.getStringMap());
ObjectNode openApi = OpenApiConverter.create().config(config).convertToNode(MODEL);
NodePointer.parse("/components/schemas/Merged").getValue(openApi).expectObjectNode();
logger.removeHandler(handler);

Assertions.assertTrue(handler.found);
}
}

0 comments on commit 8634843

Please sign in to comment.