Skip to content

Commit 669b770

Browse files
authored
Centralize protobuf scalar initialization (#1313)
## Why Built-in scalar defaults and final protobuf validation are split between the factory and scalar extension. ## What - Centralize built-in protobuf mappings and initialization in `ScalarTypeExtension`. - Keep the factory responsible only for extending the scalar and forwarding its configuration block. - Preserve the existing default, user configuration, and validation order, including renamed built-in scalars. ## Validation - `script/run_gem_specs elasticgraph-proto_ingestion` - `script/lint elasticgraph-proto_ingestion` - `script/type_check` ## Risk assessment Low. This is a structural refactor that preserves existing scalar configuration behavior. ## References - #1080 (comment) - Follow-up to #1080
1 parent 48add3e commit 669b770

5 files changed

Lines changed: 36 additions & 33 deletions

File tree

elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/factory_extension.rb

Lines changed: 2 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -18,23 +18,6 @@ module ProtoIngestion
1818
module SchemaDefinition
1919
# Extension module applied to Factory to add proto support.
2020
module FactoryExtension
21-
# Default protobuf types applied to ElasticGraph's built-in scalar types as they are constructed.
22-
BUILT_IN_SCALAR_PROTO_TYPES_BY_NAME = {
23-
"Boolean" => "bool",
24-
"Cursor" => "string",
25-
"Date" => "string",
26-
"DateTime" => "string",
27-
"Float" => "double",
28-
"ID" => "string",
29-
"Int" => "int32",
30-
"JsonSafeLong" => "int64",
31-
"LocalTime" => "string",
32-
"LongString" => "int64",
33-
"String" => "string",
34-
"TimeZone" => "string",
35-
"Untyped" => "string"
36-
}.freeze
37-
3821
# Creates a new enum type with proto extensions.
3922
#
4023
# @param name [String] enum type name
@@ -92,13 +75,9 @@ def new_object_type(name)
9275
def new_scalar_type(name)
9376
super(name) do |type|
9477
extended_type = type.extend(SchemaElements::ScalarTypeExtension) # : ::ElasticGraph::SchemaDefinition::SchemaElements::ScalarType & SchemaElements::ScalarTypeExtension
95-
96-
if (proto_type = BUILT_IN_SCALAR_PROTO_TYPES_BY_NAME[name.to_s])
97-
extended_type.protobuf type: proto_type
78+
extended_type.initialize_proto_extension do
79+
yield extended_type if block_given?
9880
end
99-
100-
yield extended_type if block_given?
101-
extended_type.finalize_protobuf_configuration!
10281
end
10382
end
10483

elasticgraph-proto_ingestion/lib/elastic_graph/proto_ingestion/schema_definition/schema_elements/scalar_type_extension.rb

Lines changed: 28 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,23 @@ module SchemaDefinition
1414
module SchemaElements
1515
# Extends ScalarType with proto field type conversion.
1616
module ScalarTypeExtension
17+
# Default protobuf types applied to ElasticGraph's built-in scalar types as they are constructed.
18+
BUILT_IN_SCALAR_PROTO_TYPES_BY_NAME = {
19+
"Boolean" => "bool",
20+
"Cursor" => "string",
21+
"Date" => "string",
22+
"DateTime" => "string",
23+
"Float" => "double",
24+
"ID" => "string",
25+
"Int" => "int32",
26+
"JsonSafeLong" => "int64",
27+
"LocalTime" => "string",
28+
"LongString" => "int64",
29+
"String" => "string",
30+
"TimeZone" => "string",
31+
"Untyped" => "string"
32+
}.freeze
33+
1734
# Configured protobuf type (e.g. string, int64, bool).
1835
# @dynamic protobuf_type
1936
attr_reader :protobuf_type
@@ -26,13 +43,20 @@ def protobuf(type:)
2643
@protobuf_type = type
2744
end
2845

29-
# Validates that a protobuf type has been configured on this scalar type. GraphQL-only
30-
# scalar types are skipped because they are not part of ingestion.
46+
# Applies any built-in protobuf type, yields for further configuration, and validates the result.
3147
#
48+
# @yield additional scalar type configuration
3249
# @return [void]
33-
# @raise [Errors::SchemaError] when missing
34-
def finalize_protobuf_configuration!
50+
# @raise [Errors::SchemaError] when a protobuf type is missing
51+
def initialize_proto_extension
52+
original_name = type_ref.with_reverted_override.name
53+
if (proto_type = BUILT_IN_SCALAR_PROTO_TYPES_BY_NAME[original_name])
54+
protobuf type: proto_type
55+
end
56+
57+
yield
3558
return if graphql_only?
59+
3660
proto_name
3761
nil
3862
end

elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/factory_extension.rbs

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,6 @@ module ElasticGraph
22
module ProtoIngestion
33
module SchemaDefinition
44
module FactoryExtension: ::ElasticGraph::SchemaDefinition::Factory
5-
BUILT_IN_SCALAR_PROTO_TYPES_BY_NAME: ::Hash[::String, ::String]
6-
75
def new_enum_type: (::String) ?{ (::ElasticGraph::SchemaDefinition::SchemaElements::EnumType & SchemaElements::EnumTypeExtension) -> void } -> (::ElasticGraph::SchemaDefinition::SchemaElements::EnumType & SchemaElements::EnumTypeExtension)
86
def new_enum_value: (::String, ::String) ?{ (::ElasticGraph::SchemaDefinition::SchemaElements::EnumValue & SchemaElements::EnumValueExtension) -> void } -> (::ElasticGraph::SchemaDefinition::SchemaElements::EnumValue & SchemaElements::EnumValueExtension)
97
def new_interface_type: (::String) ?{ (::ElasticGraph::SchemaDefinition::SchemaElements::InterfaceType & SchemaElements::ObjectInterfaceAndUnionExtension) -> void } -> (::ElasticGraph::SchemaDefinition::SchemaElements::InterfaceType & SchemaElements::ObjectInterfaceAndUnionExtension)

elasticgraph-proto_ingestion/sig/elastic_graph/proto_ingestion/schema_definition/schema_elements/scalar_type_extension.rbs

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,10 +3,12 @@ module ElasticGraph
33
module SchemaDefinition
44
module SchemaElements
55
module ScalarTypeExtension: ::ElasticGraph::SchemaDefinition::SchemaElements::ScalarType
6+
BUILT_IN_SCALAR_PROTO_TYPES_BY_NAME: ::Hash[::String, ::String]
7+
68
attr_reader protobuf_type: ::String?
79

810
def protobuf: (type: ::String) -> void
9-
def finalize_protobuf_configuration!: () -> void
11+
def initialize_proto_extension: () { () -> void } -> void
1012
def proto_name: () -> ::String
1113
def proto_type_reference: (::String package_name) -> ::String
1214
def to_proto: (::String package_name) -> nil

elasticgraph-proto_ingestion/spec/unit/elastic_graph/proto_ingestion/schema_definition/api_extension_spec.rb

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -35,16 +35,16 @@ module SchemaDefinition
3535
field_types = []
3636
proto = define_proto_schema do |s|
3737
s.object_type "Widget" do |t|
38-
FactoryExtension::BUILT_IN_SCALAR_PROTO_TYPES_BY_NAME.each_key do |type_name|
38+
SchemaElements::ScalarTypeExtension::BUILT_IN_SCALAR_PROTO_TYPES_BY_NAME.each_key do |type_name|
3939
t.field type_name.downcase, type_name
4040
end
4141
field_types = t.graphql_fields_by_name.values.map { |field| field.type.name }
4242
t.index "widgets"
4343
end
4444
end
4545

46-
expect(field_types).to match_array(FactoryExtension::BUILT_IN_SCALAR_PROTO_TYPES_BY_NAME.keys)
47-
FactoryExtension::BUILT_IN_SCALAR_PROTO_TYPES_BY_NAME.each.with_index(1) do |(type_name, proto_type), field_number|
46+
expect(field_types).to match_array(SchemaElements::ScalarTypeExtension::BUILT_IN_SCALAR_PROTO_TYPES_BY_NAME.keys)
47+
SchemaElements::ScalarTypeExtension::BUILT_IN_SCALAR_PROTO_TYPES_BY_NAME.each.with_index(1) do |(type_name, proto_type), field_number|
4848
expect(proto).to include("#{proto_type} #{type_name.downcase} = #{field_number};")
4949
end
5050
end

0 commit comments

Comments
 (0)