Refactor index_data.painless script and params#1207
Conversation
myronmarston
left a comment
There was a problem hiding this comment.
Looking good! Left some minor suggestions.
| ); | ||
| } | ||
|
|
||
| Number maybeDocVersion = source.__versions.get(params.relationship)?.get(params.sourceId); |
There was a problem hiding this comment.
On the surface, it looks like there may be a risk of a null pointer exception here--previously we did map?.get(params.sourceId) which would evaluate to null if map was null; nwo we do map.get(sourceId) which will throw an NPE if map is null.
But looking at the setup function, it looks like we initialize source.__versions[relationship] if it's not already set, so I'm thinking it's impossible for the map to be null here.
Can you confirm that analysis matches your own?
|
|
||
| source.id = params.id; | ||
| void recordSource(Map source, String id, String relationship, String sourceId, long eventVersion) { | ||
| source.id = id; |
There was a problem hiding this comment.
Setting source.id doesn't feel like it should be part of recordSource. Can we move this into applyTopLevelFields? After all, source.id = id is setting the top-level id field.
| def self.merge_list_counts_into(params, mapping:, list_counts_field_paths_for_source:) | ||
| # Here we compute the counts of our list elements so that we can index it. | ||
| data = compute_list_counts_of(params.fetch("data"), CountAccumulator.new_parent( | ||
| data = compute_list_counts_of(params.fetch("topLevelFields"), CountAccumulator.new_parent( |
There was a problem hiding this comment.
Worth renaming data = to top_level_fields =?
| ) | ||
|
|
||
| without_id_or_data = params.except("id", "data") | ||
| without_id_or_data = params.except("id", "topLevelFields") |
There was a problem hiding this comment.
| without_id_or_data = params.except("id", "topLevelFields") | |
| without_id_or_top_level_fields = params.except("id", "topLevelFields") |
(And you'll need to update the reference to it below).
| end | ||
|
|
||
| def params_for(doc_id:, event:, prepared_record:) | ||
| data = data_params.to_h do |name, param| |
There was a problem hiding this comment.
Worth renaming data = to top_level_fields = ?
Summary
This is a preparatory refactoring of the
index_dataPainless update script to make it ready for nestedsourced_fromsupport. No behavior change.params.data→params.topLevelFieldsto clarify intent (sinceputAllonly applies top-level fields)Script structure after refactoring
Why rename params.data → params.topLevelFields?
When nested
sourced_fromsupport is added, the script will also receiveparams.nestedFields(data that gets stored in__nested_sourced_dataand applied to nested elements). RenamingdatatotopLevelFieldsnow makes the distinction clear and avoids a confusing rename later when both params coexist.