Conversation
allister-beamable
left a comment
There was a problem hiding this comment.
The principle seems sound! I am not too familiar with the nuts and bolts of this code, so I defer to Chris and Dias for that part of the review :)
|
Lightbeam link |
|
Lightbeam link |
DiasAtBeamable
left a comment
There was a problem hiding this comment.
The code looks good to me. But I cannot confirm whether some of the changes will affect how the ClientCode is generated, primarily on the Unreal side. @PedroRauizBeamable has some Unreal project that would be good to test it. I have a sample C# Microservice Code with a lot of callables to test a bunch of types of usages. Let me know if you want it, I can send it to you.
There was a problem hiding this comment.
Have you tested those changes in an Unreal and Unity project to see if all the ClientCode is being properly generated?
|
@DiasAtBeamable if you can send it it would be great |
|
Lightbeam link |
…t` is directory path
…iSchemaPopulation # Conflicts: # cli/cli/Services/SwaggerService.cs
There was a problem hiding this comment.
Pull request overview
This pull request resolves issue #4407 by improving the OpenAPI schema generation flow to prevent duplicate type additions to dictionaries and enhance schema population with complete type information for nested objects and arrays. The changes introduce a new dependency tracking mechanism using a HashSet<Type> parameter that is threaded through the schema conversion process to identify and recursively add all required type definitions.
Changes:
- Refactored
SchemaGenerator.Convert()to accept aref HashSet<Type> requiredTypesparameter for tracking type dependencies - Added
ToOpenApiSchemasDictionary()andTryAddMissingSchemaTypes()methods to handle schema deduplication and recursive type resolution - Simplified type name handling by consolidating
GetGenericSanitizedFullName()andGetGenericQualifiedTypeName()into a singleGetSanitizedFullName()method - Enhanced dictionary type detection with new
IsDictionary(),IsSubclassOfRawGeneric(), andGetDictionaryTypes()helper methods - Added min/max constraints and improved type coverage for primitive types (char, sbyte, ushort, uint, ulong)
- Added comprehensive documentation to
MicroserviceRuntimeMetadataandFederationComponentMetadataclasses - Fixed malformed XML documentation in
ExpressionParser.cs - Improved CLI UX by handling directory paths in the download command
- Enhanced SwaggerService schema merging with extension comparison and path equality checks
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
SchemaGenerator.cs |
Core refactoring to add dependency tracking and recursive type resolution |
TypeExtensions.cs |
Simplified and unified type name generation methods |
ServiceDocGenerator.cs |
Integration of new schema generation flow with dependency tracking |
TypeTests.cs |
Updated all tests to use new API signature with requiredTypes parameter |
ExtraTests.cs |
Added comprehensive test coverage for various type scenarios |
MicroserviceRuntimeMetadata.cs |
Added XML documentation for all properties |
UnrealSourceGenerator.cs |
Added fallback lookup for URL-encoded schema titles |
SwaggerService.cs |
Enhanced schema merging and comparison logic |
DownloadOpenAPICommand.cs |
Improved handling of directory paths as output |
ExpressionParser.cs (both) |
Fixed malformed XML documentation tag |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| var das= GetDictionaryTypes(x); | ||
| return new OpenApiSchema | ||
| { | ||
| Type = "object", | ||
| AdditionalPropertiesAllowed = true, | ||
|
|
||
| AdditionalProperties = Convert(das.Value.ValueType, ref requiredTypes,depth - 1, sanitizeGenericType), |
There was a problem hiding this comment.
Potential null reference exception. The GetDictionaryTypes method returns a nullable value, but the code accesses das.Value without checking if das is null. If GetDictionaryTypes returns null, this will throw a NullReferenceException. Add a null check or handle the case where the dictionary types cannot be determined.
| /// <param name="oapiTypes"></param> | ||
| /// <param name="requiredTypes"></param> |
There was a problem hiding this comment.
Incomplete documentation. The XML documentation for this method lacks detailed parameter descriptions. Add proper descriptions for the 'oapiTypes' and 'requiredTypes' parameters to explain their purpose, e.g., 'oapiTypes' is the list of types to convert to schemas, and 'requiredTypes' is a set that will be populated with types referenced but not yet converted.
| /// <param name="oapiTypes"></param> | |
| /// <param name="requiredTypes"></param> | |
| /// <param name="oapiTypes">The list of OAPI types whose underlying CLR types will be converted into OpenAPI schemas.</param> | |
| /// <param name="requiredTypes">A set, passed by reference, that will be populated with additional CLR types referenced by the generated schemas but not yet converted.</param> |
| case { } x when x == typeof(short): | ||
| return new OpenApiSchema { Type = "integer", Format = "int32" }; | ||
|
|
There was a problem hiding this comment.
Duplicate case for typeof(short). This case is unreachable because there's already a case for typeof(short) at lines 283-284. This unreachable code should be removed.
| case { } x when x == typeof(short): | |
| return new OpenApiSchema { Type = "integer", Format = "int32" }; | |
| return type.FullName.Split('`')[0].Replace("+","."); | ||
| return type.FullName.Replace("+","."); | ||
| } | ||
|
|
There was a problem hiding this comment.
Missing documentation. The IsBasicType extension method lacks XML documentation comments. Add documentation to explain that it checks if a type is a basic/primitive type including common value types like int, string, bool, etc.
| /// <summary> | |
| /// Determines whether the specified <see cref="Type"/> represents a basic or primitive value type, | |
| /// including common types such as <see cref="System.Int32"/>, <see cref="string"/>, <see cref="bool"/>, | |
| /// numeric types, and characters. | |
| /// </summary> | |
| /// <param name="t">The <see cref="Type"/> to evaluate.</param> | |
| /// <returns> | |
| /// <c>true</c> if the given <paramref name="t"/> is considered a basic or primitive type; otherwise, <c>false</c>. | |
| /// </returns> |
| case Type x when x.IsAssignableTo(typeof(IList)) && x.IsGenericType: | ||
| elemType = x.GetGenericArguments()[0]; | ||
| OpenApiSchema listOpenApiSchema = elemType is { IsGenericType: true } ? Convert(elemType, 1, true) : Convert(elemType, depth - 1); | ||
| OpenApiSchema listOpenApiSchema = elemType is { IsGenericType: true } ? Convert(elemType, ref requiredTypes, depth, true) : Convert(elemType, ref requiredTypes,depth - 1); |
There was a problem hiding this comment.
Inconsistent spacing. There's a missing space before 'depth' in the method call. The code has 'ref requiredTypes,depth' but should be 'ref requiredTypes, depth' for consistency with the rest of the codebase.
| if(NamedOpenApiSchema.AreEqual(schema, component.Value, out var schemaDifferences)) | ||
| continue; | ||
|
|
||
| var mergedSchema = MergeSchemasWithExtensionMerge(schema, component.Value, schemaDifferences); |
There was a problem hiding this comment.
Unused variable. The variable 'mergedSchema' is assigned the result of MergeSchemasWithExtensionMerge but is never used. The method modifies the original schema in-place (as seen in the implementation), but the assignment suggests the result should be used. Either remove the assignment or clarify the intent.
| var mergedSchema = MergeSchemasWithExtensionMerge(schema, component.Value, schemaDifferences); | |
| MergeSchemasWithExtensionMerge(schema, component.Value, schemaDifferences); |
| case { } x when x == typeof(long): | ||
| return new OpenApiSchema { Type = "integer", Format = "int64" }; | ||
| case { } x when x == typeof(ulong): | ||
| return new OpenApiSchema { Type = "integer", Format = "int64", Minimum = ulong.MinValue, Maximum = ulong.MaxValue }; |
There was a problem hiding this comment.
Invalid minimum value for ulong. The OpenAPI specification requires minimum and maximum to be of type decimal or double, but ulong.MaxValue (18446744073709551615) exceeds the maximum value that can be accurately represented in a double (approximately 2^53 - 1 for integers). This may cause precision loss or schema validation issues. Consider omitting the Maximum constraint or documenting this limitation.
| return new OpenApiSchema { Type = "integer", Format = "int64", Minimum = ulong.MinValue, Maximum = ulong.MaxValue }; | |
| return new OpenApiSchema { Type = "integer", Format = "int64", Minimum = 0 }; |
| @@ -190,52 +237,89 @@ public static IEnumerable<Type> Traverse(Type runtimeType) | |||
| yield return runtimeType; | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Missing documentation. The TryAddMissingSchemaTypes method lacks XML documentation comments. Add documentation to explain that it recursively adds schema definitions for all types in the requiredTypes set and their dependencies to the OpenAPI document.
| /// <summary> | |
| /// Recursively adds OpenAPI schema definitions for all types in the specified | |
| /// <paramref name="requiredTypes"/> set and any additional dependent types | |
| /// discovered during schema generation to the provided <see cref="OpenApiDocument"/>. | |
| /// </summary> | |
| /// <param name="oapiDoc"> | |
| /// The <see cref="OpenApiDocument"/> whose <see cref="OpenApiComponents.Schemas"/> | |
| /// collection will be populated with schema definitions for the required types. | |
| /// </param> | |
| /// <param name="requiredTypes"> | |
| /// A set of types that must have schema definitions present in the OpenAPI document. | |
| /// Any additional types identified as dependencies while generating schemas are also | |
| /// processed until no new schema types are required. | |
| /// </param> | |
| /// <returns> | |
| /// <c>true</c> when all required types and their dependencies have been processed | |
| /// and corresponding schema definitions have been added to the OpenAPI document. | |
| /// </returns> |
| case Type x when x.IsArray: | ||
| var elemType = x.GetElementType(); | ||
| OpenApiSchema arrayOpenApiSchema = elemType is { IsGenericType: true } ? Convert(elemType, 1, true) : Convert(elemType, depth - 1); | ||
| OpenApiSchema arrayOpenApiSchema = elemType is { IsGenericType: true } ? Convert(elemType, ref requiredTypes, depth, true) : Convert(elemType, ref requiredTypes,depth - 1); |
There was a problem hiding this comment.
Inconsistent spacing. There's a missing space before 'depth' in the method call. The code has 'ref requiredTypes,depth' but should be 'ref requiredTypes, depth' for consistency with the rest of the codebase.
| @@ -244,21 +328,74 @@ public static OpenApiSchema Convert(Type runtimeType, int depth = 1, bool saniti | |||
| { | |||
| Type = "object", | |||
| AdditionalPropertiesAllowed = true, | |||
| AdditionalProperties = Convert(x.GetGenericArguments()[1], depth - 1), | |||
| AdditionalProperties = Convert(x.GetGenericArguments()[1], ref requiredTypes,depth - 1, sanitizeGenericType), | |||
| Extensions = new Dictionary<string, IOpenApiExtension> | |||
| { | |||
| [MICROSERVICE_EXTENSION_BEAMABLE_TYPE_NAMESPACE] = new OpenApiString(runtimeType.Namespace), | |||
| [MICROSERVICE_EXTENSION_BEAMABLE_TYPE_NAME] = new OpenApiString(runtimeType.Name), | |||
| [MICROSERVICE_EXTENSION_BEAMABLE_TYPE_ASSEMBLY_QUALIFIED_NAME] = new OpenApiString(runtimeType.GetGenericQualifiedTypeName()), | |||
| [MICROSERVICE_EXTENSION_BEAMABLE_TYPE_ASSEMBLY_QUALIFIED_NAME] = new OpenApiString(runtimeType.GetSanitizedFullName()), | |||
| [MICROSERVICE_EXTENSION_BEAMABLE_TYPE_OWNER_ASSEMBLY] = new OpenApiString(runtimeType.Assembly.GetName().Name), | |||
| [MICROSERVICE_EXTENSION_BEAMABLE_TYPE_OWNER_ASSEMBLY_VERSION] = new OpenApiString(runtimeType.Assembly.GetName().Version.ToString()), | |||
| [MICROSERVICE_EXTENSION_BEAMABLE_FORCE_TYPE_NAME] = new OpenApiString(runtimeType.GetGenericSanitizedFullName()) | |||
| [MICROSERVICE_EXTENSION_BEAMABLE_FORCE_TYPE_NAME] = new OpenApiString(runtimeType.GetSanitizedFullName()) | |||
| } | |||
| }; | |||
| case Type x when IsDictionary(x): | |||
| var das= GetDictionaryTypes(x); | |||
| return new OpenApiSchema | |||
| { | |||
| Type = "object", | |||
| AdditionalPropertiesAllowed = true, | |||
|
|
|||
| AdditionalProperties = Convert(das.Value.ValueType, ref requiredTypes,depth - 1, sanitizeGenericType), | |||
There was a problem hiding this comment.
Inconsistent spacing. Multiple method calls have missing spaces before 'depth' parameter. The code has 'ref requiredTypes,depth' but should be 'ref requiredTypes, depth' for consistency. This pattern appears on lines 273, 275, 277, 331, and 349.
|
Lightbeam link |
Resolves #4407
Besides that it also improves OpenAPI components generation flow, so for example when we have class like
MicroserviceRuntimeMetadatawith array fieldpublic List<FederationComponentMetadata> federatedComponents = new List<FederationComponentMetadata>();it detects that it is a separate class that is and stores information about the fact that the schema should include type information forFederationComponentMetadataas well. In old version the schema for the field was storing only that:Which only informs that there is array of anything. New version gives information about items type and also stores schema for that field type.