Skip to content

Fail on missing properties.#342

Merged
nhtruong merged 6 commits intoopensearch-project:mainfrom
dblock:fail-on-additional-properties
Jun 19, 2024
Merged

Fail on missing properties.#342
nhtruong merged 6 commits intoopensearch-project:mainfrom
dblock:fail-on-additional-properties

Conversation

@dblock
Copy link
Copy Markdown
Member

@dblock dblock commented Jun 17, 2024 •
edited
Loading

Description

A large category of bugs in the schema is that some fields are missing. For example, the distribution filed in the response from GET / was added in #336.

We should have detected the missing field in tests. Had the info schema had additionalProperties: false the schema validation would have failed with "data/version must NOT have additional properties". However adding that option means that our published schema cannot be future-proof for when new fields are added.

This PR dynamically injects additionalProperties: ... with ajv-errors that allows to custom render an error message that contains the property name that is missing.

For now it only applies to any set of properties with a required field. Otherwise it creates some false positives on index responses with dynamic mappings.

With this change:

npm run test:spec -- --verbose --tests tests/_core/info.yaml --opensearch-insecure

FAILED  Test root endpoint. (/Users/dblock/source/opensearch-project/opensearch-api-specification/dblock-opensearch-api-specification/tests/_core/info.yaml)
    FAILED  CHAPTERS 
        FAILED  Get server info. 
            PASSED  REQUEST BODY 
            PASSED  RESPONSE STATUS 
            FAILED  RESPONSE PAYLOAD (data/version/distribution property is not defined in the spec)

You get all the errors:

FAILED  Test the creation of an ingest pipeline with a text embedding processor.
 (/Users/dblock/source/opensearch-project/opensearch-api-specification/dblock-opensearch-api-specification/tests/ingest/pipeline.yaml)
    FAILED  CHAPTERS 
        FAILED  Create ingest pipeline for text embedding. 
            PASSED  PARAMETERS 
                PASSED  id 
            FAILED  REQUEST BODY (data/processors/0/text_embedding/model_id property is not defined in the spec, data/processors/0/text_embedding/field_map property is not defined in the spec)
            PASSED  RESPONSE STATUS 
            PASSED  RESPONSE PAYLOAD 
        FAILED  Query created pipeline. 
            PASSED  PARAMETERS 
                PASSED  id 
            PASSED  REQUEST BODY 
            PASSED  RESPONSE STATUS 
            FAILED  RESPONSE PAYLOAD (data/books_pipeline/processors/0/text_embedding/model_id property is not defined in the spec, data/books_pipeline/processors/0/text_embedding/field_map property is not defined in the spec)

Issues Resolved

Closes #338.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@dblock dblock mentioned this pull request Jun 17, 2024
Closed
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Jun 17, 2024 •
edited
Loading

Changes Analysis

Commit SHA: b789df6
Comparing To SHA: 16bcb63

API Changes

Summary

└─┬Paths
  └─┬/_plugins/_ml/model_groups/{model_group_id}
    └─┬GET
      └─┬Responses
        └─┬200
          └─┬application/json
            └─┬Schema
              ├──[➕] properties (24437:15)
              └──[➕] properties (24440:15)
Document Element Total Changes Breaking Changes
paths 2 0
  • Total Changes: 2
  • Additions: 2

Report

The full API changes report is available at: https://github.com/opensearch-project/opensearch-api-specification/actions/runs/9571977030/artifacts/1614563652

API Coverage

Before After Δ
Covered (%) 483 (47.31 %) 483 (47.31 %) 0 (0 %)
Uncovered (%) 538 (52.69 %) 538 (52.69 %) 0 (0 %)
Unknown 24 24 0

@dblock dblock force-pushed the fail-on-additional-properties branch 2 times, most recently from a2797e8 to 130f86c Compare June 17, 2024 20:45
@dblock
Copy link
Copy Markdown
Member Author

dblock commented Jun 17, 2024

@nhtruong I'll write tests and document this, but would love your thoughts in the meantime please?

nhtruong
nhtruong reviewed Jun 17, 2024
View reviewed changes
@dblock dblock force-pushed the fail-on-additional-properties branch from 130f86c to b650c9d Compare June 18, 2024 12:29
@dblock dblock marked this pull request as ready for review June 18, 2024 14:02
@dblock
Copy link
Copy Markdown
Member Author

dblock commented Jun 18, 2024

@nhtruong @Xtansia this is ready for review

nhtruong
nhtruong reviewed Jun 18, 2024
View reviewed changes
nhtruong
nhtruong reviewed Jun 18, 2024
View reviewed changes
nhtruong
nhtruong reviewed Jun 18, 2024
View reviewed changes
nhtruong
nhtruong reviewed Jun 18, 2024
View reviewed changes
dblock added 4 commits June 18, 2024 16:31
@dblock
Fail on missing properties.
4b1235c 
Signed-off-by: dblock <dblock@amazon.com>
@dblock
Use logger for verbose output.
7dbf2b3 
Signed-off-by: dblock <dblock@amazon.com>
@dblock
Only require additionalProperties when required fields are present.
734e3a9 
Signed-off-by: dblock <dblock@amazon.com>
@dblock
Added created_time and last_updated_time to ml.get_model_group@200.
53e6da0 
Signed-off-by: dblock <dblock@amazon.com>
@dblock dblock force-pushed the fail-on-additional-properties branch from b8484ac to 2f91b80 Compare June 18, 2024 20:31
@dblock
Replaced JSON.stringify with to_json.
ab57a68 
Signed-off-by: dblock <dblock@amazon.com>
@dblock dblock force-pushed the fail-on-additional-properties branch 2 times, most recently from e80d5bb to 25886d8 Compare June 18, 2024 20:42
@dblock
Check for property values when augmenting schema.
b789df6 
Signed-off-by: dblock <dblock@amazon.com>
@dblock dblock force-pushed the fail-on-additional-properties branch from 25886d8 to b789df6 Compare June 18, 2024 20:45
@dblock dblock requested a review from nhtruong June 19, 2024 12:17
@nhtruong nhtruong merged commit 39afe0d into opensearch-project:main Jun 19, 2024
@dblock dblock deleted the fail-on-additional-properties branch June 19, 2024 17:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@harshavamsi harshavamsi Awaiting requested review from harshavamsi harshavamsi is a code owner

@sachetalva sachetalva Awaiting requested review from sachetalva sachetalva is a code owner

@Xtansia Xtansia Awaiting requested review from Xtansia Xtansia is a code owner

@VachaShah VachaShah Awaiting requested review from VachaShah VachaShah is a code owner

@Tokesh Tokesh Awaiting requested review from Tokesh Tokesh is a code owner

@aabeshov aabeshov Awaiting requested review from aabeshov aabeshov is a code owner

1 more reviewer

@nhtruong nhtruong nhtruong approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[FEATURE] Warn on missing fields

2 participants

@dblock @nhtruong