fix issues in generating product swagger from OAS 3.1 APIs. #13055
Conversation
dushaniw
requested review from
tharindu1st,
AnuGayan,
chamilaadhi,
Arshardh and
tgtshanika
as code owners
📝 WalkthroughWalkthroughThe pull request introduces new constants for OpenAPI specifications in the API constants file and refactors the schema processing logic. The refactoring includes making the Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant F as SchemaProcessorFactory
participant P as SchemaProcessor
participant CTX as SwaggerUpdateContext
C->>F: getProcessor(schema)
F-->>C: Processor (OpenAPI30 or OpenAPI31To30)
C->>P: convertSchema(schema)
C->>P: extractReferenceFromSchema(schema, CTX)
Assessment against linked issues
Suggested reviewers
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (14)
components/apimgt/org.wso2.carbon.apimgt.impl/src/test/java/org/wso2/carbon/apimgt/impl/definitions/OASSchemaProcessorConstants.java (1)
1-51: Improve constant naming convention for better readability and maintainability.The constants in this class follow a mix of camelCase (e.g.,
dateFormat,exampleDate) and lowercase (number,cvv) naming styles. For Java constants, it's standard practice to use UPPER_SNAKE_CASE.Consider renaming the constants to follow the standard Java naming convention for constants:
- public static final String dateFormat = "date"; - public static final String number = "number"; - public static final String cvv = "cvv"; + public static final String DATE_FORMAT = "date"; + public static final String NUMBER = "number"; + public static final String CVV = "cvv";Also, it would be helpful to add Javadoc comments for individual constants or logical groups of constants to explain their purpose and usage in tests.
components/apimgt/org.wso2.carbon.apimgt.impl/src/test/java/org/wso2/carbon/apimgt/impl/definitions/SchemaProcessorFactoryTest.java (1)
28-46: Enhance the test coverage for SchemaProcessorFactory.The test method verifies processor selection for V30 and V31, as well as error handling for null spec version, but doesn't test the case when an invalid spec version is provided.
Add a test case for an invalid spec version:
schema.setSpecVersion(null); try { SchemaProcessorFactory.getProcessor(schema); } catch (IllegalArgumentException exception) { Assert.assertNotNull(exception); } + + // Test with invalid spec version + schema.setSpecVersion(SpecVersion.V31_COMPATIBILITY); // Assuming this is not handled + try { + SchemaProcessorFactory.getProcessor(schema); + Assert.fail("Should throw IllegalArgumentException for invalid spec version"); + } catch (IllegalArgumentException exception) { + Assert.assertTrue(exception.getMessage().contains("Invalid spec version")); + } }Also, consider using JUnit's expected exception annotation or assertThrows instead of try-catch for cleaner test code.
components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/definitions/SchemaProcessor.java (2)
36-42: Enhance method documentation with more details about the SwaggerUpdateContext.The method documentation should clarify how the context is used to store references and what side effects occur during extraction.
Improve the Javadoc for extractReferenceFromSchema method:
/** * Extract references from a given schema object in OpenAPI 3.x version. * + * This method traverses the schema structure and extracts all references ($ref) it contains, + * storing them in the provided SwaggerUpdateContext. The context maintains state across multiple + * extraction calls, accumulating all unique references found in the API definition. * * @param schema Schema object * @param context Swagger update context to hold the references */
44-50: Clarify the behavior of the convertSchema method.The method documentation doesn't specify that the schema is modified in-place or explain what transformations are applied to convert between OpenAPI versions.
Enhance the Javadoc for convertSchema method:
/** * Convert the given schema from a given OpenAPI version to API product's openAPI version. As of now, the default * API Product version in 3.0.1. + * + * This method modifies the schema in-place to ensure compatibility with OpenAPI 3.0.1 specification. + * Conversion may include transforming schema properties that are valid in newer versions but not in 3.0.1, + * such as converting the 'examples' array to a single 'example' value or handling the 'types' array. * * @param schema Schema object */components/apimgt/org.wso2.carbon.apimgt.impl/src/test/java/org/wso2/carbon/apimgt/impl/definitions/OAS30ProcessorTest.java (3)
43-47: Use constants from OASSchemaProcessorConstants for consistency.The test class defines its own constants that duplicate ones already defined in OASSchemaProcessorConstants, which could lead to maintenance issues.
Reuse constants from OASSchemaProcessorConstants instead of defining duplicates:
private OpenAPI30SchemaProcessor processor; - private static final String example = "example"; - private static final String number = "number"; - private static final String dateFormat = "date"; + private static final String EXAMPLE = "example"; + // Reuse constants from OASSchemaProcessorConstants + // private static final String number = OASSchemaProcessorConstants.number; + // private static final String dateFormat = OASSchemaProcessorConstants.dateFormat;
56-65: Expand test coverage for convertSchema method.The current test only verifies basic schema conversion, but doesn't test converting more complex schemas or handling specific OpenAPI 3.1 features.
Add more test cases for the convertSchema method:
@Test public void convertComplexSchemaTest() { // Test conversion of a schema with examples array (3.1 feature) Schema<Object> schema = new Schema<>(); List<Object> examples = new ArrayList<>(); examples.add("example1"); examples.add("example2"); schema.setExamples(examples); schema.setTypes(Arrays.asList(APIConstants.OPENAPI_STRING_DATA_TYPE, APIConstants.OPENAPI_NULL_DATA_TYPE)); processor.convertSchema(schema); // Verify conversion: examples array should be converted to single example value Assert.assertEquals("example1", schema.getExample()); Assert.assertNull(schema.getExamples()); Assert.assertEquals(APIConstants.OPENAPI_STRING_DATA_TYPE, schema.getType()); Assert.assertNull(schema.getTypes()); }
1-478: Consider reorganizing test methods and helper methods for better readability.The test methods and helper methods are interspersed throughout the class, making it difficult to navigate and understand the test structure.
Reorganize the class to group:
- All test methods together
- All schema creation helper methods together
- All assertion helper methods together
This structure would improve readability and make the test class easier to maintain.
components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/definitions/OpenAPI31To30SchemaProcessor.java (2)
30-69: Ensure references are handled consistently.
TheextractReferenceFromSchemamethod diligently checks for array schemas (schema.getItems()), composed schemas (allOf,anyOf,oneOf), and mapped schemas (schema.getAdditionalProperties()), which is good. However, consider confirming that nested composed schemas withinadditionalPropertiesor arrays are also processed consistently. Nested arrays or maps might appear in practical scenarios, so it can be beneficial to verify they aren't missed.
71-102: Clarify multiple examples vs singleexampleusage.
When there are multiple examples present inschema.getExamples(), the logic sets onlyschema.setExample(firstExample). This can lead to information loss in certain schemas. If preserving all examples is a requirement, consider extending the logic or clearly documenting that only the first example is supported.components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/definitions/OASParserUtil.java (5)
157-188: Provide consistent access modifiers for the newSwaggerUpdateContext.
You’ve madeSwaggerUpdateContextpublic, which helps other classes create and use the context more flexibly. This is sensible for broader usage. Ensure you have enough encapsulation for your internal data structures (e.g.,paths,aggregatedComponents), especially if you anticipate external classes might manipulate them directly. If that’s undesirable, consider adding defensive copies or method-based accessors.
793-808: Guard against potential null items in array schemas.
TheprocessArraySchemamethod assumesschema.getItems()is not null once confirmed to be an array, but complex nested cases could still slip through. It might be good to handle any unexpected null item references or additional array validations.
822-831: Confirm property-level transformations.
Declaringprotected static void processSchemaProperties(...)allows conversions of property schemas in nested objects. Ensure that interplay with composed schemas (e.g.,allOf) is not overlooked if properties themselves can be one of multiple types. Additional coverage in tests can help confirm correctness.
833-848: Well-structured adaptors inconvertSchema.
This method usesSchemaProcessorto convert a schema according to version specs. The fallback strategy in the catch block is clear. One minor improvement is to ensure logs remain consistent (e.g., using log.warn if partial data can still be processed, vs. log.error for total failure).
859-862: FavorStringUtils.isNotEmptychecks.
getRefKeysegments the reference path reliably. It might be beneficial to confirm thatrefis not empty before splitting. The current usage seems safe, but defensive checks can help in edge cases.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/APIConstants.java(2 hunks)components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/definitions/OASParserUtil.java(6 hunks)components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/definitions/OpenAPI30SchemaProcessor.java(1 hunks)components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/definitions/OpenAPI31To30SchemaProcessor.java(1 hunks)components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/definitions/SchemaProcessor.java(1 hunks)components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/definitions/SchemaProcessorFactory.java(1 hunks)components/apimgt/org.wso2.carbon.apimgt.impl/src/test/java/org/wso2/carbon/apimgt/impl/definitions/OAS30ProcessorTest.java(1 hunks)components/apimgt/org.wso2.carbon.apimgt.impl/src/test/java/org/wso2/carbon/apimgt/impl/definitions/OAS31To30ProcessorTest.java(1 hunks)components/apimgt/org.wso2.carbon.apimgt.impl/src/test/java/org/wso2/carbon/apimgt/impl/definitions/OASSchemaProcessorConstants.java(1 hunks)components/apimgt/org.wso2.carbon.apimgt.impl/src/test/java/org/wso2/carbon/apimgt/impl/definitions/SchemaProcessorFactoryTest.java(1 hunks)
🧰 Additional context used
🧬 Code Definitions (6)
components/apimgt/org.wso2.carbon.apimgt.impl/src/test/java/org/wso2/carbon/apimgt/impl/definitions/SchemaProcessorFactoryTest.java (1)
components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/definitions/SchemaProcessorFactory.java (1)
SchemaProcessorFactory(33-60)
components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/definitions/SchemaProcessorFactory.java (1)
components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/APIConstants.java (1)
APIConstants(32-3343)
components/apimgt/org.wso2.carbon.apimgt.impl/src/test/java/org/wso2/carbon/apimgt/impl/definitions/OAS31To30ProcessorTest.java (2)
components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/APIConstants.java (1)
APIConstants(32-3343)components/apimgt/org.wso2.carbon.apimgt.impl/src/test/java/org/wso2/carbon/apimgt/impl/definitions/OASSchemaProcessorConstants.java (1)
OASSchemaProcessorConstants(24-51)
components/apimgt/org.wso2.carbon.apimgt.impl/src/test/java/org/wso2/carbon/apimgt/impl/definitions/OAS30ProcessorTest.java (2)
components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/APIConstants.java (1)
APIConstants(32-3343)components/apimgt/org.wso2.carbon.apimgt.impl/src/test/java/org/wso2/carbon/apimgt/impl/definitions/OASSchemaProcessorConstants.java (1)
OASSchemaProcessorConstants(24-51)
components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/definitions/OpenAPI31To30SchemaProcessor.java (2)
components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/APIConstants.java (1)
APIConstants(32-3343)components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/definitions/OASParserUtil.java (1)
OASParserUtil(132-2271)
components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/definitions/OASParserUtil.java (1)
components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/definitions/SchemaProcessorFactory.java (1)
SchemaProcessorFactory(33-60)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build-product (4, group4)
- GitHub Check: build-product (3, group3)
- GitHub Check: build-product (2, group2)
- GitHub Check: build-product (1, group1)
- GitHub Check: run-benchmark-test
- GitHub Check: build-carbon
🔇 Additional comments (27)
components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/definitions/SchemaProcessorFactory.java (3)
35-36: Use of static final singletons is well-structured.
Having these as private constants ensures reusability and prevents redundant instantiation.
38-40: Private constructor enforces the factory pattern.
This design prevents direct instantiation and centralizes object creation.
48-59: Version validation logic is concise and correct.
Throws an exception for unsupported specifications, ensuring only recognized versions (3.0 or 3.1) proceed.components/apimgt/org.wso2.carbon.apimgt.impl/src/test/java/org/wso2/carbon/apimgt/impl/definitions/OAS31To30ProcessorTest.java (10)
42-45: Initialization with a real processor.
Using a concreteOpenAPI31To30SchemaProcessorinstance provides meaningful test coverage.
47-62: Example handling tests.
Both methods verify correct behavior for new vs. existing examples, ensuring consistent conversion results.
64-75: Nullable type checks.
Recognizes"null"in the types set, settingschema.setNullable(true)appropriately.
77-96: Single vs. multiple types.
Confirms the processor sets the type if exactly one is present and skips if more than one exists—a logical approach.
98-115: Type override and empty examples.
Ensures existing schema type remains unchanged and no spurious example is set when examples are empty.
117-121: Null schema tolerance.
CallingconvertSchema(null)with no adverse effects indicates safe handling of edge cases.
123-235: Reference extraction for arrays, maps, and allOf.
Tests thoroughly validate that references are correctly identified and added to the context.
237-301: Complex composed schemas.
OneOf, anyOf, and allOf with nested arrays are verified. The coverage is very comprehensive.
303-310: Nested references in arrays.
Ensures multiple references in array items are captured correctly.
312-634: Helper methods for complex fixtures.
These generator methods reduce repetition and enhance clarity by providing standardized schema objects for testing.components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/definitions/OpenAPI30SchemaProcessor.java (5)
36-73: Robust schema-handling logic.
Appropriately processes array, map, composed, and object schemas, funneling references into the context.
75-101: Nested schema references.
Traverses property schemas to collect references, effectively handling deeper structures.
103-130: Composed schema processing.
Manages allOf, anyOf, oneOf thoroughly, and logs an error when none is found—straightforward error handling.
132-154: Composed schema detail work.
Branches for object vs. array vs. direct$refusage to ensure accurate reference tracking in deeper compositions.
156-164: No-op conversion for OpenAPI 3.0.
Leaving 3.0 schemas unchanged aligns with the stated requirement that only 3.1 requires down-conversion.components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/APIConstants.java (2)
1707-1707: New constant added for OpenAPI 3.0 version representation.The constant
OAS_V30has been added to represent the OpenAPI Specification version 3.0, using the value "v30". This complements the existingOAS_V31constant and follows the same naming convention.
1737-1740: Added essential OpenAPI data type constants.These constants define the standard data types used in OpenAPI specifications:
OPENAPI_OBJECT_DATA_TYPEfor the "object" typeOPENAPI_ARRAY_DATA_TYPEfor the "array" typeOPENAPI_STRING_DATA_TYPEfor the "string" typeOPENAPIV31_SCHEMA_TYPE_NULLABLEfor the OpenAPI 3.1 specific "null" typeThese constants will be used in schema processing and conversion between OpenAPI 3.0 and 3.1, ensuring consistency when handling different schema types.
components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/definitions/OpenAPI31To30SchemaProcessor.java (3)
104-133: Validate nested schema references.
TheextractReferenceFromNestedSchemamethod callsOASParserUtil.convertSchema(schema)early on, which helps unify schema structure. The recursion for composed or embedded array schemas is then delegated to other utility methods. This approach looks correct. Consider adding more unit tests for mixed scenarios, e.g., when both properties andallOf/anyOfoccur together.
135-162: Confirm assumption of non-nested composed schemas.
The doc comment indicates this method assumes no nested arrays inallOf,anyOf, oroneOf. However,processComposedSchema(sc, updateContext, references)further callsextractReferenceFromNestedSchemaorOASParserUtil.processArraySchema, indicating partial support for nested scenarios. To avoid confusion, clarify whether or not nesting is fully supported, or add error logs if deeper nesting is out of scope.
164-188: Enhance null checks inprocessComposedSchema.
The method gracefully handles references and properties. Still, verify if additional safeguards or checks need to be applied to handle unusual edge cases (e.g.,sc.getType()returning null). Overall, this is well-structured. Good job.components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/definitions/OASParserUtil.java (4)
728-729: Confirm schema conversion for parameters.
Lines 728-729 convert the parameter schema to align with the target OpenAPI version. This ensures references and types are consistent throughout. Verify that any existing examples or composed schemas within parameters are also processed in the same pass.
775-791: Good separation of concerns inextractReferenceFromSchema.
The method delegates the actual extraction to a suitableSchemaProcessor, providing a clean, version-agnostic facade. Exception handling is concise, defaulting to a safe skip on errors. Overall, this is a sound pattern.
850-857: Efficient lookups inaddToReferenceObjectMap.
This snippet looks up a category inreferenceObjectMapand appends the reference key. The approach is straightforward. If performance becomes a concern with large sets of references, consider short-circuit checks or concurrency-safety if references can be added from multiple threads. For now, this appears sufficient.
864-876: Validate component category assumptions.
This method parses the reference string to identify which components section (schemas,parameters, etc.) the reference belongs to. If new OAS 3.1 references or custom vendor extension references appear, consider either gracefully ignoring them or logging a warning. The logic is clear otherwise.
...on.apimgt.impl/src/test/java/org/wso2/carbon/apimgt/impl/definitions/OAS30ProcessorTest.java
Outdated
Show resolved
Hide resolved
...on.apimgt.impl/src/test/java/org/wso2/carbon/apimgt/impl/definitions/OAS30ProcessorTest.java
Outdated
Show resolved
Hide resolved
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #13055 +/- ##
============================================
- Coverage 31.94% 31.91% -0.03%
+ Complexity 10037 3947 -6090
============================================
Files 2141 2144 +3
Lines 158634 158726 +92
Branches 21741 21759 +18
============================================
- Hits 50672 50658 -14
- Misses 101980 102072 +92
- Partials 5982 5996 +14
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
components/apimgt/org.wso2.carbon.apimgt.impl/src/test/java/org/wso2/carbon/apimgt/impl/definitions/OAS30ProcessorTest.java (2)
76-76: Consider reversing the arguments inassertEquals()for clarity.
In JUnit, the first argument is conventionally the expected value and the second is the actual value. For example, useAssert.assertEquals(1, context.getReferenceObjectMapping().get(OASSchemaProcessorConstants.SCHEMAS).size())instead ofAssert.assertEquals(context.getReferenceObjectMapping().get(OASSchemaProcessorConstants.SCHEMAS).size(), 1).Also applies to: 148-148
140-156: Suggestion: validate property-by-property extraction inextractReferenceFromComplexObjectSchemaTest.
Although the test already checks references, consider explicitly verifying that each nested property in the composed schema has the expected data type. This could enhance clarity for future maintainers.components/apimgt/org.wso2.carbon.apimgt.impl/src/test/java/org/wso2/carbon/apimgt/impl/definitions/OAS31To30ProcessorTest.java (1)
123-128: Minor style improvement forassertEquals().
As in the other file, consider reversing the arguments to haveAssert.assertEquals(expected, actual)for clarity (e.g.,Assert.assertEquals(1, context.getReferenceObjectMapping().…size())).components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/definitions/OpenAPI30SchemaProcessor.java (2)
82-101: Potential missed case for nestedMapSchemainextractReferenceFromNestedSchema.
Currently, nested schemas in properties are recognized if they areComposedSchema,ObjectSchema, orArraySchema, but not explicitly if they areMapSchema. Consider adding the same logic forMapSchemato handle references in nested maps.
127-129: Non-blocking note on composed schema logging.
The warning about an unidentified schema is helpful. However, if the code might still handle a nested scenario differently, consider clarifying what is expected or referencing relevant documentation for future maintainers.components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/definitions/OASParserUtil.java (1)
775-790: Consider adding unit tests for the schema processing implementation.The new schema processing functionality is critical for handling different OpenAPI versions. Comprehensive unit tests would help ensure that the implementations correctly handle various schema formats, edge cases, and conversion scenarios.
Also applies to: 793-810, 835-849
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/definitions/OASParserUtil.java(6 hunks)components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/definitions/OpenAPI30SchemaProcessor.java(1 hunks)components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/definitions/OpenAPI31To30SchemaProcessor.java(1 hunks)components/apimgt/org.wso2.carbon.apimgt.impl/src/test/java/org/wso2/carbon/apimgt/impl/definitions/OAS30ProcessorTest.java(1 hunks)components/apimgt/org.wso2.carbon.apimgt.impl/src/test/java/org/wso2/carbon/apimgt/impl/definitions/OAS31To30ProcessorTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/definitions/OpenAPI31To30SchemaProcessor.java
🧰 Additional context used
🧬 Code Definitions (4)
components/apimgt/org.wso2.carbon.apimgt.impl/src/test/java/org/wso2/carbon/apimgt/impl/definitions/OAS30ProcessorTest.java (2)
components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/APIConstants.java (1)
APIConstants(32-3343)components/apimgt/org.wso2.carbon.apimgt.impl/src/test/java/org/wso2/carbon/apimgt/impl/definitions/OASSchemaProcessorConstants.java (1)
OASSchemaProcessorConstants(24-52)
components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/definitions/OpenAPI30SchemaProcessor.java (2)
components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/APIConstants.java (1)
APIConstants(32-3343)components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/definitions/OASParserUtil.java (1)
OASParserUtil(132-2276)
components/apimgt/org.wso2.carbon.apimgt.impl/src/test/java/org/wso2/carbon/apimgt/impl/definitions/OAS31To30ProcessorTest.java (2)
components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/APIConstants.java (1)
APIConstants(32-3343)components/apimgt/org.wso2.carbon.apimgt.impl/src/test/java/org/wso2/carbon/apimgt/impl/definitions/OASSchemaProcessorConstants.java (1)
OASSchemaProcessorConstants(24-52)
components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/definitions/OASParserUtil.java (1)
components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/definitions/SchemaProcessorFactory.java (1)
SchemaProcessorFactory(33-60)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build-product (2, group2)
- GitHub Check: build-product (4, group4)
- GitHub Check: build-product (3, group3)
- GitHub Check: run-benchmark-test
- GitHub Check: build-carbon
- GitHub Check: build-product (1, group1)
🔇 Additional comments (17)
components/apimgt/org.wso2.carbon.apimgt.impl/src/test/java/org/wso2/carbon/apimgt/impl/definitions/OAS30ProcessorTest.java (2)
50-61: Good coverage of nullable and example fields inconvertSchemaTest.
This test thoroughly checks thatconvertSchema(schema)retains or sets expected values (examples, types, etc.).
106-122: Thorough test for reference extraction inextractReferenceFromComposedArrayWithRef.
This ensures composed arrays withanyOfandoneOfreferences are handled correctly, validating that all three references are captured. Excellent job.components/apimgt/org.wso2.carbon.apimgt.impl/src/test/java/org/wso2/carbon/apimgt/impl/definitions/OAS31To30ProcessorTest.java (4)
47-53: Great test verifying example selection inconvertSchemaShouldSetExampleWhenExamplesExist.
This confirms the first example is used if multiple examples are present. The logic is clear and robust.
65-74: Commendable handling of 'null' in types.
Settingschema.setNullable(true)and removing"null"fromschema.getTypes()is an excellent demonstration of correct OAS 3.1-to-3.0 conversion.
207-231: Good coverage inextractReferenceFromComplexObjectSchemaTest.
The checks for multiple references, composed schemas, and array properties ensure that the 3.1-to-3.0 logic is robust.
248-262: Excellent nested arrays test inextractReferenceFromNestedArrayInsideOneOfSchema.
It successfully validates multiple levels of array nesting, ensuring the references are extracted properly.components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/definitions/OpenAPI30SchemaProcessor.java (2)
44-73: Overall logic inextractReferenceFromSchemalooks good.
- Correctly short-circuits when
schema == null.- Handles
$refvs non-$refschemas.- Processes arrays, maps, composed schemas, and object schemas distinctly.
No critical issues found here.
161-163: No operation needed inconvertSchema.
Given that the schema remains in 3.0 form, leavingconvertSchemaempty is consistent. This approach is acceptable and matches the code comments.components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/definitions/OASParserUtil.java (9)
157-157: Visibility change to SwaggerUpdateContext class is appropriate and necessary.Changing the visibility of
SwaggerUpdateContextfrom package-private to public allows for proper integration with the new schema processor architecture. This enables the schema processors to access and manipulate the context effectively.
728-730: Schema conversion added to parameter processing.The call to
convertSchemaensures schemas are properly converted to the appropriate format before reference extraction, which is crucial for supporting multiple OpenAPI versions, particularly when generating product Swagger from OAS 3.1 APIs.
775-790: Well-structured reference extraction method with proper error handling.This new public method delegates to appropriate schema processors based on the OpenAPI version, decoupling the extraction logic from the utility class itself. The try-catch block properly handles invalid schema specifications, enhancing the robustness of the code.
793-810: Specialized handling for array schemas improves processing.The dedicated method for processing array schemas provides consistent schema conversion and proper reference handling for array items. This specialized approach ensures that complex nested structures are handled correctly.
824-826: Schema conversion added to property processing.Adding
convertSchemato theprocessSchemaPropertiesmethod ensures that schema properties are properly converted before processing, maintaining consistency with the overall schema processing approach.
835-849: Robust schema conversion implementation.This new method intelligently delegates schema conversion to the appropriate processor based on the schema's OpenAPI version. The error handling is appropriate, logging issues without throwing exceptions that might disrupt the overall processing flow.
852-859: Visibility adjustments to support new architecture.Changing the visibility of
addToReferenceObjectMapto protected allows subclasses and related components to reuse this functionality, which is essential for the modular schema processing approach.
861-867: Enhanced utility method with proper null handling.The
getRefKeymethod now includes proper handling for empty strings, improving robustness. Making it protected allows reuse in subclasses and related components.
869-881: Component category extraction supports schema processor architecture.Making this method protected allows it to be used by the schema processor implementations, ensuring consistent handling of component references across the system.
Fix wso2/api-manager#3750