Skip to content

Commit 4c60a60

Browse files
committed
Address review feedback on JSON ingestion spec migration
- Port the JSON-schema tests from `rake_tasks_spec` (version bump enforcement, versioned metadata maintenance, renamed/deleted field/type guidance, conflict and routing/rollover deletion errors) into `schema_artifact_manager_extension_spec` in place of the minimal from-scratch tests, restoring the original coverage. - Rewrite `wrappers_spec` to drive the wrappers through the public schema definition API instead of exercising the internal classes directly. - Fix the value semantics of the stateless field type wrappers (`Scalar`, `Enum`, `Union`): `DelegateClass` defines `==` to unwrap only the left operand, so two wrappers around equal field types never compared equal even though their `hash` values matched, breaking the `eql?`/`hash` contract. A shared `ValueSemantics` module now unwraps both sides, keeping `==`/`eql?`/`hash` consistent.
1 parent 55df0d7 commit 4c60a60

19 files changed

Lines changed: 894 additions & 408 deletions

File tree

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -90,7 +90,7 @@ def new_scalar_type(name)
9090
extended_type.json_schema(**options)
9191
end
9292

93-
yield extended_type if block_given?
93+
yield extended_type
9494
extended_type.finalize_json_schema_configuration!
9595
end
9696
end
@@ -104,7 +104,7 @@ def new_scalar_indexing_field_type(scalar_type:)
104104
def new_type_with_subfields(schema_kind, name, wrapping_type:, field_factory:)
105105
super(schema_kind, name, wrapping_type: wrapping_type, field_factory: field_factory) do |type|
106106
extended_type = type.extend(SchemaElements::TypeWithSubfieldsExtension) # : ::ElasticGraph::SchemaDefinition::SchemaElements::TypeWithSubfields & SchemaElements::TypeWithSubfieldsExtension
107-
yield extended_type if block_given?
107+
yield extended_type
108108
end
109109
end
110110

elasticgraph-json_ingestion/lib/elastic_graph/json_ingestion/schema_definition/indexing/field_type/enum.rb

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
# frozen_string_literal: true
88

99
require "delegate"
10+
require "elastic_graph/json_ingestion/schema_definition/indexing/field_type/value_semantics"
1011
require "elastic_graph/schema_definition/indexing/field_type/enum"
1112

1213
module ElasticGraph
@@ -19,6 +20,10 @@ module FieldType
1920
#
2021
# @private
2122
class Enum < DelegateClass(ElasticGraph::SchemaDefinition::Indexing::FieldType::Enum)
23+
prepend ValueSemantics
24+
25+
# @dynamic __getobj__
26+
2227
# @return [Hash<String, ::Object>] additional ElasticGraph metadata to put in the JSON schema for this enum type.
2328
def json_schema_field_metadata_by_field_name
2429
{}

elasticgraph-json_ingestion/lib/elastic_graph/json_ingestion/schema_definition/indexing/field_type/scalar.rb

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
# frozen_string_literal: true
88

99
require "delegate"
10+
require "elastic_graph/json_ingestion/schema_definition/indexing/field_type/value_semantics"
1011
require "elastic_graph/schema_definition/indexing/field_type/scalar"
1112
require "elastic_graph/support/hash_util"
1213

@@ -19,6 +20,10 @@ module FieldType
1920
#
2021
# @private
2122
class Scalar < DelegateClass(ElasticGraph::SchemaDefinition::Indexing::FieldType::Scalar)
23+
prepend ValueSemantics
24+
25+
# @dynamic __getobj__
26+
2227
# @return [Hash] empty hash, as scalar types have no subfields
2328
def json_schema_field_metadata_by_field_name
2429
{}

elasticgraph-json_ingestion/lib/elastic_graph/json_ingestion/schema_definition/indexing/field_type/union.rb

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
# frozen_string_literal: true
88

99
require "delegate"
10+
require "elastic_graph/json_ingestion/schema_definition/indexing/field_type/value_semantics"
1011
require "elastic_graph/schema_definition/indexing/field_type/union"
1112

1213
module ElasticGraph
@@ -18,6 +19,10 @@ module FieldType
1819
#
1920
# @private
2021
class Union < DelegateClass(ElasticGraph::SchemaDefinition::Indexing::FieldType::Union)
22+
prepend ValueSemantics
23+
24+
# @dynamic __getobj__
25+
2126
# @return [Hash] empty hash, as union types have no subfields
2227
def json_schema_field_metadata_by_field_name
2328
{}
Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
1+
# Copyright 2024 - 2026 Block, Inc.
2+
#
3+
# Use of this source code is governed by an MIT-style
4+
# license that can be found in the LICENSE file or at
5+
# https://opensource.org/licenses/MIT.
6+
#
7+
# frozen_string_literal: true
8+
9+
module ElasticGraph
10+
module JSONIngestion
11+
module SchemaDefinition
12+
module Indexing
13+
module FieldType
14+
# Provides value-equality semantics for the JSON-schema-aware field type wrappers that delegate
15+
# to a wrapped core field type without adding any state of their own (`Scalar`, `Enum`, `Union`).
16+
#
17+
# `DelegateClass` defines `==` so that it unwraps only the *left* operand before comparing, which
18+
# means `wrapper == equivalent_wrapper` compares the wrapped object against the right-hand
19+
# *wrapper* (rather than against its wrapped object) and is therefore never equal--even though
20+
# `hash` delegates to the wrapped object and reports them equal. That inconsistency breaks the
21+
# `eql?`/`hash` contract and causes `Set`/`Hash`/`uniq` de-duplication to treat equivalent
22+
# wrappers as distinct. Here we unwrap both sides so two wrappers around equal objects compare
23+
# equal, keeping `==`/`eql?`/`hash` consistent. (`FieldType::Object` solves the same problem with
24+
# its own implementation because it carries additional JSON schema state in its equality.)
25+
#
26+
# @private
27+
module ValueSemantics
28+
# @param other [Object] the object to compare against
29+
# @return [Boolean] true when `other` wraps an equal field type (or is the wrapped field type itself)
30+
def ==(other)
31+
case other
32+
when ValueSemantics
33+
__getobj__ == other.__getobj__
34+
else
35+
super
36+
end
37+
end
38+
39+
def eql?(other)
40+
self == other
41+
end
42+
43+
# @return [Integer] a hash code derived from the wrapped field type
44+
def hash
45+
__getobj__.hash
46+
end
47+
end
48+
end
49+
end
50+
end
51+
end
52+
end

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,14 +13,14 @@ module ElasticGraph
1313
subfields: ::Array[Indexing::Field],
1414
mapping_options: ::ElasticGraph::SchemaDefinition::Mixins::HasTypeInfo::optionsHash
1515
) -> Indexing::FieldType::Object
16-
def new_scalar_type: (::String) ?{ (::ElasticGraph::SchemaDefinition::SchemaElements::ScalarType & SchemaElements::ScalarTypeExtension) -> void } -> (::ElasticGraph::SchemaDefinition::SchemaElements::ScalarType & SchemaElements::ScalarTypeExtension)
16+
def new_scalar_type: (::String) { (::ElasticGraph::SchemaDefinition::SchemaElements::ScalarType & SchemaElements::ScalarTypeExtension) -> void } -> (::ElasticGraph::SchemaDefinition::SchemaElements::ScalarType & SchemaElements::ScalarTypeExtension)
1717
def new_scalar_indexing_field_type: (scalar_type: ::ElasticGraph::SchemaDefinition::SchemaElements::ScalarType) -> Indexing::FieldType::Scalar
1818
def new_type_with_subfields: (
1919
::ElasticGraph::SchemaDefinition::SchemaElements::schemaKind,
2020
::String,
2121
wrapping_type: ::ElasticGraph::SchemaDefinition::SchemaElements::anyObjectType,
2222
field_factory: ::Method
23-
) ?{ (::ElasticGraph::SchemaDefinition::SchemaElements::TypeWithSubfields & SchemaElements::TypeWithSubfieldsExtension) -> void } -> (::ElasticGraph::SchemaDefinition::SchemaElements::TypeWithSubfields & SchemaElements::TypeWithSubfieldsExtension)
23+
) { (::ElasticGraph::SchemaDefinition::SchemaElements::TypeWithSubfields & SchemaElements::TypeWithSubfieldsExtension) -> void } -> (::ElasticGraph::SchemaDefinition::SchemaElements::TypeWithSubfields & SchemaElements::TypeWithSubfieldsExtension)
2424
def new_union_indexing_field_type: (::Hash[::String, Indexing::FieldType::Object]) -> Indexing::FieldType::Union
2525
def new_results: () -> ::ElasticGraph::SchemaDefinition::Results
2626
def new_schema_artifact_manager: (**untyped) -> ::ElasticGraph::SchemaDefinition::SchemaArtifactManager

elasticgraph-json_ingestion/sig/elastic_graph/json_ingestion/schema_definition/indexing/field_type/enum.rbs

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,9 +8,12 @@ module ElasticGraph
88
end
99

1010
class Enum < EnumSupertype
11+
include ValueSemantics
12+
1113
def json_schema_field_metadata_by_field_name: () -> ::Hash[::String, JSONSchemaFieldMetadata]
1214
def format_field_json_schema_customizations: (::Hash[::String, untyped]) -> ::Hash[::String, untyped]
1315
def to_json_schema: () -> ::Hash[::String, untyped]
16+
def __getobj__: () -> ::ElasticGraph::SchemaDefinition::Indexing::FieldType::Enum
1417
end
1518
end
1619
end

elasticgraph-json_ingestion/sig/elastic_graph/json_ingestion/schema_definition/indexing/field_type/scalar.rbs

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,9 +8,12 @@ module ElasticGraph
88
end
99

1010
class Scalar < ScalarSupertype
11+
include ValueSemantics
12+
1113
def json_schema_field_metadata_by_field_name: () -> ::Hash[::String, JSONSchemaFieldMetadata]
1214
def format_field_json_schema_customizations: (::Hash[::String, untyped]) -> ::Hash[::String, untyped]
1315
def to_json_schema: () -> ::Hash[::String, untyped]
16+
def __getobj__: () -> ::ElasticGraph::SchemaDefinition::Indexing::FieldType::Scalar
1417
end
1518
end
1619
end

elasticgraph-json_ingestion/sig/elastic_graph/json_ingestion/schema_definition/indexing/field_type/union.rbs

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,11 +8,14 @@ module ElasticGraph
88
end
99

1010
class Union < UnionSupertype
11+
include ValueSemantics
12+
1113
def self.new: (::ElasticGraph::SchemaDefinition::Indexing::FieldType::Union) -> instance
1214

1315
def json_schema_field_metadata_by_field_name: () -> ::Hash[::String, JSONSchemaFieldMetadata]
1416
def format_field_json_schema_customizations: (::Hash[::String, untyped]) -> ::Hash[::String, untyped]
1517
def to_json_schema: () -> ::Hash[::String, untyped]
18+
def __getobj__: () -> ::ElasticGraph::SchemaDefinition::Indexing::FieldType::Union
1619
end
1720
end
1821
end
Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
module ElasticGraph
2+
module JSONIngestion
3+
module SchemaDefinition
4+
module Indexing
5+
module FieldType
6+
module ValueSemantics : _Delegator
7+
def ==: (untyped other) -> bool
8+
def eql?: (untyped other) -> bool
9+
def hash: () -> ::Integer
10+
end
11+
12+
interface _Delegator
13+
def __getobj__: () -> untyped
14+
15+
# Provided by `DelegateClass`; `ValueSemantics#==` calls `super` to fall back to it.
16+
def ==: (untyped other) -> bool
17+
end
18+
end
19+
end
20+
end
21+
end
22+
end

0 commit comments

Comments
 (0)