Skip to content

Commit

Permalink
#8 Stack overflow on recursive schemas fix
Browse files Browse the repository at this point in the history
  • Loading branch information
galovics committed Dec 9, 2018
1 parent c3982ca commit 6031c9d
Show file tree
Hide file tree
Showing 8 changed files with 613 additions and 9 deletions.
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
package io.redskap.swagger.brake.core.model.transformer;

import static java.util.stream.Collectors.toList;
import static org.apache.commons.lang3.StringUtils.isNotBlank;

import java.util.Collections;
import java.util.List;
import java.util.Map;
import java.util.*;

import io.redskap.swagger.brake.core.model.Schema;
import io.redskap.swagger.brake.core.model.SchemaAttribute;
Expand All @@ -14,7 +13,6 @@
import io.swagger.v3.oas.models.media.ArraySchema;
import lombok.RequiredArgsConstructor;
import org.apache.commons.collections4.CollectionUtils;
import org.apache.commons.lang3.StringUtils;
import org.springframework.stereotype.Component;

@Component
Expand All @@ -24,7 +22,11 @@ public class SchemaTransformer implements Transformer<io.swagger.v3.oas.models.m

@Override
public Schema transform(io.swagger.v3.oas.models.media.Schema from) {
return internalTransform(from);
try {
return internalTransform(from);
} finally {
SeenRefHolder.clear();
}
}

private Schema internalTransform(io.swagger.v3.oas.models.media.Schema swSchema) {
Expand All @@ -40,8 +42,10 @@ private Schema internalTransform(io.swagger.v3.oas.models.media.Schema swSchema)
}

private Schema transformSchema(io.swagger.v3.oas.models.media.Schema swSchema) {
if (!StringUtils.isBlank(swSchema.get$ref())) {
io.swagger.v3.oas.models.media.Schema resolvedSchema = getSchemaForRef(swSchema.get$ref());
String ref = swSchema.get$ref();
if (isNotBlank(ref) && SeenRefHolder.isNotSeen(ref)) {
io.swagger.v3.oas.models.media.Schema resolvedSchema = getSchemaForRef(ref);
SeenRefHolder.store(ref);
return internalTransform(resolvedSchema);
}

Expand Down Expand Up @@ -80,4 +84,36 @@ private io.swagger.v3.oas.models.media.Schema getSchemaForRef(String originalRef
}
return schemaStore.get(refName).orElseThrow(() -> new IllegalStateException("Reference not found for " + refName));
}

/*
* The purpose of this class is to keep track of already seen schema references to avoid recursive schemas breaking the functionality.
*/
private static class SeenRefHolder {
private static final ThreadLocal<Collection<String>> HOLDER = new ThreadLocal<>();

private static Collection<String> get() {
Collection<String> seenRefs = HOLDER.get();
if (seenRefs == null) {
seenRefs = new HashSet<>();
HOLDER.set(seenRefs);
}
return seenRefs;
}

static boolean isSeen(String refName) {
return get().contains(refName);
}

static boolean isNotSeen(String refName) {
return !isSeen(refName);
}

static void store(String refName) {
get().add(refName);
}

static void clear() {
HOLDER.remove();
}
}
}
Original file line number Diff line number Diff line change
@@ -1,16 +1,17 @@
package io.redskap.swagger.brake.integration;
package io.redskap.swagger.brake.integration.nobreakingchange;

import static org.assertj.core.api.Assertions.assertThat;

import java.util.Collection;

import io.redskap.swagger.brake.core.BreakingChange;
import io.redskap.swagger.brake.integration.AbstractSwaggerBrakeIntTest;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.springframework.test.context.junit4.SpringRunner;

@RunWith(SpringRunner.class)
public class NoBreakingChangeTest extends AbstractSwaggerBrakeIntTest {
public class NoBreakingChangeIntTest extends AbstractSwaggerBrakeIntTest {
@Test
public void testNoBreakingChangeWorksCorrectly() {
// given
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
package io.redskap.swagger.brake.integration.nobreakingchange;

import static org.assertj.core.api.Assertions.assertThat;

import java.util.Collection;

import io.redskap.swagger.brake.core.BreakingChange;
import io.redskap.swagger.brake.integration.AbstractSwaggerBrakeIntTest;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.springframework.test.context.junit4.SpringRunner;

@RunWith(SpringRunner.class)
public class RecursiveSchemaIntTest extends AbstractSwaggerBrakeIntTest {
@Test
public void testNoBreakingChangeWorksCorrectly() {
// given
String oldApiPath = "nobreakingchange/recursive/schema.json";
String newApiPath = "nobreakingchange/recursive/schema_v2.json";
// when
Collection<BreakingChange> result = execute(oldApiPath, newApiPath);
// then
assertThat(result).isEmpty();
}
}

Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
package io.redskap.swagger.brake.integration.response;

import static org.assertj.core.api.Assertions.assertThat;

import java.util.Collection;
import java.util.Collections;

import io.redskap.swagger.brake.core.BreakingChange;
import io.redskap.swagger.brake.core.model.HttpMethod;
import io.redskap.swagger.brake.core.rule.response.ResponseTypeAttributeRemovedBreakingChange;
import io.redskap.swagger.brake.integration.AbstractSwaggerBrakeIntTest;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.springframework.test.context.junit4.SpringRunner;

@RunWith(SpringRunner.class)
public class RecursiveTypeAttributeRemovedIntTest extends AbstractSwaggerBrakeIntTest {
@Test
public void testResponseTypeChangeIsBreakingChangeWhenExistingAttributeRemoved() {
// given
String oldApiPath = "response/recursiveresponseattributeremoved/schema.json";
String newApiPath = "response/recursiveresponseattributeremoved/schema_v2.json";
ResponseTypeAttributeRemovedBreakingChange bc1 =
new ResponseTypeAttributeRemovedBreakingChange("/api/v1/audits/summary/{businessId}", HttpMethod.GET, "200", "unverifiedPayoffBreakdown.amountApplied");
Collection<BreakingChange> expected = Collections.singletonList(bc1);
// when
Collection<BreakingChange> result = execute(oldApiPath, newApiPath);
// then
assertThat(result).hasSize(1);
assertThat(result).hasSameElementsAs(expected);
}
}
128 changes: 128 additions & 0 deletions src/test/resources/nobreakingchange/recursive/schema.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,128 @@
{
"swagger": "2.0",
"host": "localhost",
"basePath": "/",
"tags": [
{
"name": "audit-controller",
"description": "Audit Controller"
},
{
"name": "audit-fee-controller",
"description": "Audit Fee Controller"
},
{
"name": "operation-handler",
"description": "Operation Handler"
}
],
"paths": {
"/api/v1/audits/summary/{businessId}": {
"get": {
"tags": [
"audit-controller"
],
"summary": "fetchAuditSummaryByBusinessId",
"operationId": "fetchAuditSummaryByBusinessIdUsingGET",
"produces": [
"*/*"
],
"parameters": [
{
"name": "businessId",
"in": "path",
"description": "businessId",
"required": true,
"type": "string",
"format": "uuid"
}
],
"responses": {
"200": {
"description": "OK",
"schema": {
"$ref": "#/definitions/AuditSummaryDTO"
}
},
"401": {
"description": "Unauthorized"
},
"403": {
"description": "Forbidden"
},
"404": {
"description": "Not Found"
}
},
"security": [
{
"jwt": [
"global"
]
}
],
"deprecated": false
}
}
},
"securityDefinitions": {
"jwt": {
"type": "apiKey",
"name": "Authorization",
"in": "header"
}
},
"definitions": {
"AuditSummaryDTO": {
"type": "object",
"properties": {
"payoffSavings": {
"type": "number"
},
"unverifiedPayoff": {
"type": "number"
},
"unverifiedPayoffBreakdown": {
"type": "array",
"items": {
"$ref": "#/definitions/PaymentItemDTO"
}
},
"unverifiedVehicles": {
"type": "integer",
"format": "int64"
}
},
"title": "AuditSummaryDTO"
},
"PaymentItemDTO": {
"type": "object",
"properties": {
"amountApplied": {
"type": "number"
},
"children": {
"type": "array",
"items": {
"$ref": "#/definitions/PaymentItemDTO"
}
},
"description": {
"type": "string"
},
"recordType": {
"type": "string",
"enum": [
"PRINCIPAL",
"INTEREST",
"CPP",
"TRANSPORTATION_FEE",
"AUDIT_FEE",
"OTHER_FEE"
]
}
},
"title": "PaymentItemDTO"
}
}
}
Loading

0 comments on commit 6031c9d

Please sign in to comment.