Skip to content

Allow indexed_shape-only geo_shape and xy_shape queries - #1161

Open
Tejashribambal19 wants to merge 1 commit into
opensearch-project:mainfrom
Tejashribambal19:fix-1160-geoshape-required-shape
Open

Allow indexed_shape-only geo_shape and xy_shape queries#1161
Tejashribambal19 wants to merge 1 commit into
opensearch-project:mainfrom
Tejashribambal19:fix-1160-geoshape-required-shape

Conversation

@Tejashribambal19

Copy link
Copy Markdown

Summary

Fixes #1160.

The current API specification marks shape as required in both GeoShapeQueryField and XyShapeQueryField. However, OpenSearch supports pre-indexed shape queries using indexed_shape without an accompanying shape, as documented.

Because this specification is the source of truth for generated clients, the current requirement prevents clients from constructing valid indexed_shape-only queries.

This PR removes the shape requirement from both schemas, allowing both inline shape queries and pre-indexed indexed_shape queries.

Changes

  • Remove required: [shape] from GeoShapeQueryField
  • Remove required: [shape] from XyShapeQueryField

Motivation

The OpenSearch documentation describes shape and indexed_shape as alternative ways to define a geo/xy shape query. Requiring shape makes valid pre-indexed shape queries fail schema validation and propagates incorrect constraints to generated clients.

This change aligns the API specification with the documented query behavior.

Testing

  • Verified that the schema changes are limited to GeoShapeQueryField and XyShapeQueryField.
  • Existing query behavior remains unchanged for inline shape queries.

Notes

The current specification tests cover inline shape queries but do not include coverage for the indexed_shape path. This PR focuses on correcting the schema; regression tests for indexed_shape can be added separately if maintainers prefer additional coverage.

@lsh1215 lsh1215 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for picking this up, @Tejashribambal19 — I opened the linked issue #1160. This unblocks the immediate case; I left two inline notes suggesting we model exactly one of shape/indexed_shape (via the existing TermsLookup oneOf pattern) instead of dropping required, since the contract is "one or the other, not both."

One more gap: there's no test coverage for the indexed_shape path yet — the existing geo_shape_* / xy_shape_* stories only exercise inline shape (open question #4 in the issue). I have a branch with the oneOf modeling plus geo_shape_indexed_shape / xy_shape_indexed_shape stories, validated against the merged schema (lint:spec / merge / unit all green). Happy to fold those into this PR or open an alternative — whichever you and the maintainers prefer.

$ref: '#/components/schemas/GeoShape'
relation:
$ref: '_common.yaml#/components/schemas/GeoShapeRelation'
required:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dropping required unblocks indexed_shape-only, but it also makes both shape+indexed_shape and neither valid. Per the docs and @xluo-aws (opensearch-java#2011), it should be exactly one — already modeled in this same file by TermsLookup:

allOf:
  - oneOf:
      - type: object
        properties:
          indexed_shape: { $ref: '#/components/schemas/FieldLookup' }
        required: [indexed_shape]
      - type: object
        properties:
          shape: { $ref: '#/components/schemas/GeoShape' }
        required: [shape]

$ref: '#/components/schemas/XyShape'
relation:
$ref: '_common.yaml#/components/schemas/GeoShapeRelation'
required:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here — XyShapeQueryField needs the same exactly-one allOf/oneOf treatment (with XyShape instead of GeoShape).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] shape should not be required in GeoShapeQueryField (blocks pre-indexed shape queries in generated clients)

2 participants