Preserve highlighting for returnable: false fields#1120
Conversation
2d41661 to
e283639
Compare
There was a problem hiding this comment.
Good catch--I hadn't considered how highlightable interacts here. I agree that a returnable: false, highlightable: true field should behave as you've demonstrated here. However, I think it's reasonable for field.highlightable? to return false for a returnable: false field. That is: if the caller hasn't passed highlightable: ... at all, I think returnable: false should make the field non-highlightable.
After all, the primary reason to pass returnable: false is to achieve storage savings, and it would be a bit silly/frustrating for a user to pass returnable: false (without knowing they also need to pass highlightable: false) and have it not provide any storage savings. And the various -able? predicates on Field are meant to provide intelligent defaulting. sortable?, groupable?, aggregatable?, and highlightable? all have intelligent defaulting logic. highlightable? can also have additional logic.
BTW, it feels like #1108 is incomplete without the schema def updates you've added here--so, if it's easy to do, consider moving the schema def updates from this PR to that one, the artifact updates from this to #1119, and then this can just be the acceptance test. (But it's also ok in the form you have it.)
| elsif object_type | ||
| object_type.source_excludes_paths(path_prefix: "#{path}.") | ||
| if object_type | ||
| if non_returnable && !field.highlightable? |
There was a problem hiding this comment.
What if field isn't highlightable but one or more of its subfields are highlightable? I'm not quite sure what the behavior should be in that case, but we should consider it. (And tests should cover it).
Edit: we should probably look at how the other -able? fields behave when they return false for a parent field and true for a child field--we'd want consistency with how that's treated.
| object_type.source_excludes_paths( | ||
| path_prefix: "#{path}.", | ||
| under_non_returnable_parent: non_returnable | ||
| ) |
There was a problem hiding this comment.
I often like keyword arguments (particularly to avoid issues related to passing args in the wrong order) but since the only caller of this method which passes args is a recursive call from within the method...it's easy to see the method signature above. So in this case I think I'd prefer to make these positional args and collapse it to one line:
| object_type.source_excludes_paths( | |
| path_prefix: "#{path}.", | |
| under_non_returnable_parent: non_returnable | |
| ) | |
| object_type.source_excludes_paths("#{path}.", non_returnable) |
9aebf65 to
cef0449
Compare
5f6abd3 to
a66b825
Compare
cef0449 to
a2d2d8b
Compare
a66b825 to
81e87d8
Compare
Summary
joshauw/returnable-widgets-schemaparent branchreturnable: falsefields by only excluding non-highlightable hidden fields from_source_source.excludesbehaviorTesting
bundle exec rspec elasticgraph-schema_definition/spec/unit/elastic_graph/schema_definition/datastore_config/index_mappings/miscellaneous_spec.rbbundle exec rspec elasticgraph-schema_definition/spec/unit/elastic_graph/schema_definition/datastore_config/index_mappings/miscellaneous_spec.rbNO_VCR=1 bundle exec rspec elasticgraph-graphql/spec/acceptance/returnable_fields_spec.rbscript/lint elasticgraph-schema_definition/lib/elastic_graph/schema_definition/mixins/has_indices.rb elasticgraph-schema_definition/spec/unit/elastic_graph/schema_definition/datastore_config/index_mappings/miscellaneous_spec.rb elasticgraph-graphql/spec/acceptance/returnable_fields_spec.rb