Namespace JSON schema helpers under JSON ingestion#1203
Open
jwils wants to merge 1 commit into
Open
Conversation
7bc0045 to
6267113
Compare
461ba24 to
64ff2af
Compare
6267113 to
040cea2
Compare
040cea2 to
e6a4787
Compare
64ff2af to
7d4becc
Compare
e6a4787 to
eb21230
Compare
| "id" => { | ||
| "description" => "The unique identifier of the record.", | ||
| "type" => "string", | ||
| "maxLength" => ElasticGraph::DEFAULT_MAX_KEYWORD_LENGTH |
Collaborator
There was a problem hiding this comment.
Suggested change
| "maxLength" => ElasticGraph::DEFAULT_MAX_KEYWORD_LENGTH | |
| "maxLength" => DEFAULT_MAX_KEYWORD_LENGTH |
Now that the outer module is ElasticGraph and not ElasticGraph::JSONIngestion, this can resolve without the ElasticGraph prefix.
Can you apply this throughout the PR?
| :missing_necessary_fields | ||
| ) | ||
| def json_schema_version | ||
| json_schema.fetch(ElasticGraph::JSON_SCHEMA_VERSION_KEY) |
Collaborator
There was a problem hiding this comment.
Suggested change
| json_schema.fetch(ElasticGraph::JSON_SCHEMA_VERSION_KEY) | |
| json_schema.fetch(JSON_SCHEMA_VERSION_KEY) |
| old_type_name_by_current_name = {} # : ::Hash[::String, ::String] | ||
|
|
||
| defs = json_schema.fetch("$defs").to_h do |type_name, type_def| | ||
| if type_name != ElasticGraph::EVENT_ENVELOPE_JSON_SCHEMA_NAME && (properties = type_def["properties"]) |
Collaborator
There was a problem hiding this comment.
Suggested change
| if type_name != ElasticGraph::EVENT_ENVELOPE_JSON_SCHEMA_NAME && (properties = type_def["properties"]) | |
| if type_name != EVENT_ENVELOPE_JSON_SCHEMA_NAME && (properties = type_def["properties"]) |
| field_type: field_type, | ||
| fully_qualified_path: field_path.fully_qualified_path_in_index | ||
| ) | ||
| def identify_missing_necessary_fields_for_index_def(object_type, index_def, json_schema_resolver) |
Collaborator
There was a problem hiding this comment.
Funny that we had this unused argument...
| private | ||
|
|
||
| def json_schema_with_metadata_merger: () -> Indexing::JSONSchemaWithMetadata::Merger | ||
| def json_schema_with_metadata_merger: () -> ::ElasticGraph::JSONIngestion::SchemaDefinition::Indexing::JSONSchemaWithMetadata::Merger |
Collaborator
There was a problem hiding this comment.
Suggested change
| def json_schema_with_metadata_merger: () -> ::ElasticGraph::JSONIngestion::SchemaDefinition::Indexing::JSONSchemaWithMetadata::Merger | |
| def json_schema_with_metadata_merger: () -> JSONIngestion::SchemaDefinition::Indexing::JSONSchemaWithMetadata::Merger |
| @current_public_json_schema: ::Hash[::String, untyped]? | ||
| @latest_versioned_json_schema: ::Hash[::String, untyped]? | ||
| @json_schema_with_metadata_merger: Indexing::JSONSchemaWithMetadata::Merger? | ||
| @json_schema_with_metadata_merger: ::ElasticGraph::JSONIngestion::SchemaDefinition::Indexing::JSONSchemaWithMetadata::Merger? |
Collaborator
There was a problem hiding this comment.
Suggested change
| @json_schema_with_metadata_merger: ::ElasticGraph::JSONIngestion::SchemaDefinition::Indexing::JSONSchemaWithMetadata::Merger? | |
| @json_schema_with_metadata_merger: JSONIngestion::SchemaDefinition::Indexing::JSONSchemaWithMetadata::Merger? |
| def mapping: () -> ::Hash[::String, untyped] | ||
| def json_schema: () -> ::Hash[::String, untyped] | ||
| def json_schema_metadata: () -> JSONSchemaFieldMetadata | ||
| def json_schema_metadata: () -> ::ElasticGraph::JSONIngestion::SchemaDefinition::Indexing::JSONSchemaFieldMetadata |
Collaborator
There was a problem hiding this comment.
Suggested change
| def json_schema_metadata: () -> ::ElasticGraph::JSONIngestion::SchemaDefinition::Indexing::JSONSchemaFieldMetadata | |
| def json_schema_metadata: () -> JSONIngestion::SchemaDefinition::Indexing::JSONSchemaFieldMetadata |
The ::ElasticGraph:: prefix shouldn't be needed in any of these RBS files since you're within module ElasticGraph. Can you apply this to all the RBS files?
Comment on lines
+241
to
+244
| # @private | ||
| class MissingNecessaryField < ::Data | ||
| # @dynamic initialize, with, field_type, fully_qualified_path | ||
| end |
Collaborator
There was a problem hiding this comment.
Suggested change
| # @private | |
| class MissingNecessaryField < ::Data | |
| # @dynamic initialize, with, field_type, fully_qualified_path | |
| end |
Doesn't seem like this is needed?
7d4becc to
e64b589
Compare
eb21230 to
2afdcd4
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
After the move-only PR preserves rename detection, put the moved helper constants under the
ElasticGraph::JSONIngestionnamespace.What
Risk Assessment
Medium - namespace-only behavior should be unchanged, but it updates call sites across schema_definition.
References