From 17f91943eba9a6290e84ed61c52e0114466153fb Mon Sep 17 00:00:00 2001 From: Paul Sturgess Date: Fri, 17 Nov 2023 16:53:21 +0000 Subject: [PATCH] Generate partial objects as component schemas (#23) The generated clients try to be smart and "re-use" schemas where possible. This makes for some really odd documentation and for go and php, some really odd types. e.g in the load balancer response, it says `certificates` property is type `GetDataCenters200ResponseDataCentersInnerCountry` which has nothing to do with the certificates. It's just that it uses the same properties. By "partial object" we mean that we return only some of the properties in the API response. e.g. we might sometimes only return the id and name properties of a `VirtualMachine`. Now we don't generate these type of schemas "inline" within the response. Actually this is recommended by Swagger anyway. And then we also no-longer even generate an inline object within any schema. The reason again is because of the way the generators attempt to "optimize" and re-use other schemas already defined. This does make for some verbose schema ids, such as: `PostLoadBalancerRules200ResponseLoadBalancerRuleLoadBalancer` Which is named as such because it needs to be unique: ``` PostLoadBalancerRules (the endpoint) 200Response (the type of response) LoadBalancerRuleLoadBalancer which follows the way the params are named and nested: { load_balancer_rule: { load_balancer: } } ``` Bill and I took a look at if there were any alternative solutions for these definitions within the openapi spec, but couldn't find any :( The upside is it simplifies our generator code. closes: https://github.com/krystal/apia-openapi/issues/22 --- lib/apia/open_api/helpers.rb | 16 +- lib/apia/open_api/objects/parameters.rb | 6 +- lib/apia/open_api/objects/request_body.rb | 6 +- lib/apia/open_api/objects/response.rb | 56 ++++--- lib/apia/open_api/objects/schema.rb | 23 ++- spec/support/fixtures/openapi.json | 169 +++++++++++++--------- 6 files changed, 148 insertions(+), 128 deletions(-) diff --git a/lib/apia/open_api/helpers.rb b/lib/apia/open_api/helpers.rb index c25e687..814ca15 100644 --- a/lib/apia/open_api/helpers.rb +++ b/lib/apia/open_api/helpers.rb @@ -7,13 +7,18 @@ module Helpers # A component schema is a re-usable schema that can be referenced by other parts of the spec # e.g. { "$ref": "#/components/schemas/PaginationObject" } - def add_to_components_schemas(definition) - id = generate_id_from_definition(definition.type.klass.definition) + def add_to_components_schemas(definition, id, **schema_opts) return unless @spec.dig(:components, :schemas, id).nil? component_schema = {} @spec[:components][:schemas][id] = component_schema - Objects::Schema.new(spec: @spec, definition: definition, schema: component_schema).add_to_spec + Objects::Schema.new( + spec: @spec, + definition: definition, + schema: component_schema, + id: id, + **schema_opts + ).add_to_spec end def convert_type_to_open_api_data_type(type) @@ -28,8 +33,9 @@ def convert_type_to_open_api_data_type(type) end end - def generate_schema_ref(definition) - id = generate_id_from_definition(definition) + def generate_schema_ref(definition, id: nil, **schema_opts) + id ||= generate_id_from_definition(definition.type.klass.definition) + add_to_components_schemas(definition, id, **schema_opts) { "$ref": "#/components/schemas/#{id}" } end diff --git a/lib/apia/open_api/objects/parameters.rb b/lib/apia/open_api/objects/parameters.rb index d0d28a5..c6b5927 100644 --- a/lib/apia/open_api/objects/parameters.rb +++ b/lib/apia/open_api/objects/parameters.rb @@ -37,8 +37,7 @@ def add_to_spec generate_argument_set_params elsif @argument.array? if @argument.type.enum? || @argument.type.object? - items = generate_schema_ref(@argument.type.klass.definition) - add_to_components_schemas(@argument) + items = generate_schema_ref(@argument) else items = { type: convert_type_to_open_api_data_type(@argument.type) } end @@ -57,11 +56,10 @@ def add_to_spec param = { name: @argument.name.to_s, in: "query", - schema: generate_schema_ref(@argument.type.klass.definition) + schema: generate_schema_ref(@argument) } param[:required] = true if @argument.required? add_to_parameters(param) - add_to_components_schemas(@argument) else param = { name: @argument.name.to_s, diff --git a/lib/apia/open_api/objects/request_body.rb b/lib/apia/open_api/objects/request_body.rb index ab46b6b..0e73571 100644 --- a/lib/apia/open_api/objects/request_body.rb +++ b/lib/apia/open_api/objects/request_body.rb @@ -36,8 +36,7 @@ def add_to_spec required << arg.name.to_s if arg.required? if arg.array? if arg.type.argument_set? || arg.type.enum? - items = generate_schema_ref(arg.type.klass.definition) - add_to_components_schemas(arg) + items = generate_schema_ref(arg) else items = { type: convert_type_to_open_api_data_type(arg.type) } end @@ -47,8 +46,7 @@ def add_to_spec items: items } elsif arg.type.argument_set? || arg.type.enum? - @properties[arg.name.to_s] = generate_schema_ref(arg.type.klass.definition) - add_to_components_schemas(arg) + @properties[arg.name.to_s] = generate_schema_ref(arg) else # scalar @properties[arg.name.to_s] = { type: convert_type_to_open_api_data_type(arg.type) diff --git a/lib/apia/open_api/objects/response.rb b/lib/apia/open_api/objects/response.rb index 8bf7138..7ae6048 100644 --- a/lib/apia/open_api/objects/response.rb +++ b/lib/apia/open_api/objects/response.rb @@ -34,6 +34,7 @@ def initialize(spec:, path_ids:, route:, route_spec:) @route = route @endpoint = route.endpoint @route_spec = route_spec + @http_status = @endpoint.definition.http_status end def add_to_spec @@ -45,7 +46,7 @@ def add_to_spec response_schema[:required] = required_fields.keys if required_fields.any? @route_spec[:responses] = { - "#{@endpoint.definition.http_status}": { + "#{@http_status}": { description: @endpoint.definition.description || "", content: { "application/json": { @@ -88,42 +89,32 @@ def build_properties_for_polymorph(field_name, field, properties) if field_includes_all_properties?(field) refs = [] field.type.klass.definition.options.map do |_, polymorph_option| - refs << generate_schema_ref(polymorph_option.type.klass.definition) - add_to_components_schemas(polymorph_option) + refs << generate_schema_ref(polymorph_option) end properties[field_name] = { oneOf: refs } else # we assume the partially selected attributes must be present in all of the polymorph options # and that each option returns the same data type for that attribute - object_schema = {} - Objects::Schema.new( - spec: @spec, - definition: field.type.klass.definition.options.values.first, - schema: object_schema, + properties[field_name] = generate_schema_ref( + field.type.klass.definition.options.values.first, + id: generate_field_id(field_name), endpoint: @endpoint, path: [field] - ).add_to_spec - properties[field_name] = object_schema + ) end end def build_properties_for_array(field_name, field, properties) if field.type.object? || field.type.enum? if field_includes_all_properties?(field) - items = generate_schema_ref(field.type.klass.definition) - add_to_components_schemas(field) + items = generate_schema_ref(field) else - array_schema = {} - Objects::Schema.new( - spec: @spec, - definition: field, - schema: array_schema, + items = generate_schema_ref( + field, + id: generate_field_id(field_name), endpoint: @endpoint, path: [field] - ).add_to_spec - if array_schema[:properties].any? - items = array_schema - end + ) end else items = { type: convert_type_to_open_api_data_type(field.type) } @@ -138,18 +129,14 @@ def build_properties_for_array(field_name, field, properties) def build_properties_for_object_or_enum(field_name, field, properties) if field_includes_all_properties?(field) - properties[field_name] = generate_schema_ref(field.type.klass.definition) - add_to_components_schemas(field) + properties[field_name] = generate_schema_ref(field) else - object_schema = {} - Objects::Schema.new( - spec: @spec, - definition: field, - schema: object_schema, + properties[field_name] = generate_schema_ref( + field, + id: generate_field_id(field_name), endpoint: @endpoint, path: [field] - ).add_to_spec - properties[field_name] = object_schema + ) end end @@ -157,6 +144,15 @@ def field_includes_all_properties?(field) field.include.nil? end + def generate_field_id(field_name) + [ + @route_spec[:operationId].sub(":", "_").gsub(":", "").split("/"), + @http_status, + "response", + field_name + ].flatten.join("_") + end + end end end diff --git a/lib/apia/open_api/objects/schema.rb b/lib/apia/open_api/objects/schema.rb index a97b8a0..918a431 100644 --- a/lib/apia/open_api/objects/schema.rb +++ b/lib/apia/open_api/objects/schema.rb @@ -35,10 +35,11 @@ class Schema include Apia::OpenApi::Helpers - def initialize(spec:, definition:, schema:, endpoint: nil, path: nil) + def initialize(spec:, definition:, schema:, id:, endpoint: nil, path: nil) @spec = spec @definition = definition @schema = schema + @id = id @endpoint = endpoint @path = path @children = [] @@ -61,8 +62,7 @@ def build_schema_for_polymorph @schema[:properties] ||= {} refs = [] @definition.type.klass.definition.options.map do |_, polymorph_option| - refs << generate_schema_ref(polymorph_option.type.klass.definition) - add_to_components_schemas(polymorph_option) + refs << generate_schema_ref(polymorph_option) end @schema[:properties][@definition.name.to_s] = { oneOf: refs } end @@ -110,8 +110,7 @@ def generate_schema_for_child(schema, child, all_properties_included) elsif child.type.argument_set? || child.type.enum? || child.type.polymorph? schema[:type] = "object" schema[:properties] ||= {} - schema[:properties][child.name.to_s] = generate_schema_ref(child.type.klass.definition) - add_to_components_schemas(child) + schema[:properties][child.name.to_s] = generate_schema_ref(child) elsif child.type.object? generate_properties_for_object(schema, child, all_properties_included) else # scalar @@ -133,19 +132,15 @@ def generate_properties_for_object(schema, child, all_properties_included) schema[:type] = "object" schema[:properties] ||= {} if all_properties_included - schema[:properties][child.name.to_s] = generate_schema_ref(child.type.klass.definition) - add_to_components_schemas(child) + schema[:properties][child.name.to_s] = generate_schema_ref(child) else child_path = @path.nil? ? nil : @path + [child] - child_schema = {} - schema[:properties][child.name.to_s] = child_schema - self.class.new( - spec: @spec, - definition: child, - schema: child_schema, + schema[:properties][child.name.to_s] = generate_schema_ref( + child, + id: "#{@id}_#{child.name}", endpoint: @endpoint, path: child_path - ).add_to_spec + ) end end diff --git a/spec/support/fixtures/openapi.json b/spec/support/fixtures/openapi.json index b521757..605bd3d 100644 --- a/spec/support/fixtures/openapi.json +++ b/spec/support/fixtures/openapi.json @@ -153,31 +153,7 @@ "times": { "type": "array", "items": { - "type": "object", - "properties": { - "unix": { - "type": "integer" - }, - "year": { - "type": "object", - "properties": { - "as_string": { - "type": "string" - } - } - }, - "as_array_of_objects": { - "type": "array", - "items": { - "type": "object", - "properties": { - "as_integer": { - "type": "integer" - } - } - } - } - } + "$ref": "#/components/schemas/post_example_format_multiple_200_response_times" } } }, @@ -262,12 +238,7 @@ ] }, "my_partial_polymorph": { - "type": "object", - "properties": { - "number": { - "type": "integer" - } - }, + "$ref": "#/components/schemas/get_time_now_200_response_my_partial_polymorph", "nullable": true } }, @@ -351,12 +322,7 @@ ] }, "my_partial_polymorph": { - "type": "object", - "properties": { - "number": { - "type": "integer" - } - }, + "$ref": "#/components/schemas/post_time_now_200_response_my_partial_polymorph", "nullable": true } }, @@ -414,23 +380,7 @@ "schema": { "properties": { "time": { - "type": "object", - "properties": { - "unix": { - "type": "integer" - }, - "day_of_week": { - "$ref": "#/components/schemas/CoreAPI_Objects_Day" - }, - "year": { - "type": "object", - "properties": { - "as_string": { - "type": "string" - } - } - } - }, + "$ref": "#/components/schemas/get_test_object_200_response_time", "nullable": true }, "object_id": { @@ -479,23 +429,7 @@ "schema": { "properties": { "time": { - "type": "object", - "properties": { - "unix": { - "type": "integer" - }, - "day_of_week": { - "$ref": "#/components/schemas/CoreAPI_Objects_Day" - }, - "year": { - "type": "object", - "properties": { - "as_string": { - "type": "string" - } - } - } - }, + "$ref": "#/components/schemas/post_test_object_200_response_time", "nullable": true }, "object_id": { @@ -638,6 +572,39 @@ "unix" ] }, + "post_example_format_multiple_200_response_times": { + "type": "object", + "properties": { + "unix": { + "type": "integer" + }, + "year": { + "$ref": "#/components/schemas/post_example_format_multiple_200_response_times_year" + }, + "as_array_of_objects": { + "type": "array", + "items": { + "$ref": "#/components/schemas/post_example_format_multiple_200_response_times_as_array_of_objects" + } + } + } + }, + "post_example_format_multiple_200_response_times_year": { + "type": "object", + "properties": { + "as_string": { + "type": "string" + } + } + }, + "post_example_format_multiple_200_response_times_as_array_of_objects": { + "type": "object", + "properties": { + "as_integer": { + "type": "integer" + } + } + }, "CoreAPI_Objects_Time": { "type": "object", "properties": { @@ -748,6 +715,44 @@ } } }, + "get_time_now_200_response_my_partial_polymorph": { + "type": "object", + "properties": { + "number": { + "type": "integer" + } + } + }, + "post_time_now_200_response_my_partial_polymorph": { + "type": "object", + "properties": { + "number": { + "type": "integer" + } + } + }, + "get_test_object_200_response_time": { + "type": "object", + "properties": { + "unix": { + "type": "integer" + }, + "day_of_week": { + "$ref": "#/components/schemas/CoreAPI_Objects_Day" + }, + "year": { + "$ref": "#/components/schemas/get_test_object_200_response_time_year" + } + } + }, + "get_test_object_200_response_time_year": { + "type": "object", + "properties": { + "as_string": { + "type": "string" + } + } + }, "CoreAPI_ArgumentSets_ObjectLookup": { "description": "All 'object[]' params are mutually exclusive, only one can be provided.", "type": "object", @@ -759,6 +764,28 @@ "type": "string" } } + }, + "post_test_object_200_response_time": { + "type": "object", + "properties": { + "unix": { + "type": "integer" + }, + "day_of_week": { + "$ref": "#/components/schemas/CoreAPI_Objects_Day" + }, + "year": { + "$ref": "#/components/schemas/post_test_object_200_response_time_year" + } + } + }, + "post_test_object_200_response_time_year": { + "type": "object", + "properties": { + "as_string": { + "type": "string" + } + } } }, "securitySchemes": {