Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed REST status codes for RBAC and provisioning #842

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

owaiskazi19
Copy link
Member

Description

Depends on opensearch-project/flow-framework#1083

Issues Resolved

List any issues this PR will resolve, e.g. Closes [...].

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.

Copy link
Contributor

github-actions bot commented Mar 14, 2025

Changes Analysis

Commit SHA: 5ada6b9
Comparing To SHA: 5d1077e

API Changes

Summary

SUCCESS: No changes found between specifications

Report

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

API Coverage

Before After Δ
Covered (%) 663 (64.94 %) 663 (64.94 %) 0 (0 %)
Uncovered (%) 358 (35.06 %) 358 (35.06 %) 0 (0 %)
Unknown 49 49 0

status: 202
Copy link
Member

Choose a reason for hiding this comment

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

Line 3 speceifies version >=2.12. This is a change for 3.0+.

I'm not sure how we are supposed to specify a version change. Do we make line 3 >=2.12 && <3 and have a new file for >=3.0.0?

@Xtansia advice here? Developer guide doesn't cover major version API changes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is failing for 3.0.0, too.

If you want to test this on another unreleased version, you will have to add an entry to the test-spec workflow like this: https://github.com/opensearch-project/opensearch-api-specification/blob/main/.github/workflows/test-spec.yml#L69-L71

Copy link
Member Author

Choose a reason for hiding this comment

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

@nhtruong don't we need to have 3.0.0-alpha-1 rather 3.0 here? That's why it's failing for 3.0

Copy link
Collaborator

Choose a reason for hiding this comment

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

Then we'll need to update the ref pointing to the latest iteration of 3.0.0 for the test to pass if the change only applies to the newer build of 3.0.0.

Copy link
Collaborator

@nhtruong nhtruong Mar 17, 2025

Choose a reason for hiding this comment

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

It's getting much more complicated to host 2 different major versions of the spec (2.x and 3.x) and test them now (in this case this API can only be tested against 3.x because 2.x expects a 200 response, not the 202 that his PR describes). Having a branch for each major version (with main hosting the latest unreleased major version) will solve this issue. What's your thought on this?
cc: @Xtansia @dblock

Copy link
Member

@dblock dblock Mar 17, 2025

Choose a reason for hiding this comment

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

I would avoid duplicating/branching the spec because it means backporting hell like we have in so many repos. You should be able to use x-version-added and friends to specify that in 2.x the API returns 200 and in 3.x it returns 202. You could also say that the API returns either of 200/202 with a comment, which I think is ok for now.

For tests it's even easier because the version: header tells when a test runs or saying it's any of 200 or 202 is fine.

Copy link
Member

Choose a reason for hiding this comment

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

So to be clear, for this one I would make sure we support status: [200, 202] in tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @dblock! Pushed the change

Copy link
Member Author

Choose a reason for hiding this comment

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

If we change the type to array instead of integer for expected response here, that would mean changing all the responses in the test files to return an array rather an int?

Copy link
Member

Choose a reason for hiding this comment

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

You can have anyOf in the schema between an integer and an array of integers.

Signed-off-by: Owais <[email protected]>
Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

You'll need to implement support for an array of status codes to get this in. Give it a shot!

@owaiskazi19 owaiskazi19 force-pushed the fix-error-code branch 6 times, most recently from da66b31 to 9a06988 Compare March 20, 2025 20:29
Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

I'm pretty sure there's some code to write to check status, it's not just schema. Plus tests.

type: integer
description: The expected HTTP status code. Default to 200.
default: 200
anyOf:
Copy link
Member

Choose a reason for hiding this comment

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

It's one or the other, and only one, so I think oneOf is more appropriate.

Copy link
Member

Choose a reason for hiding this comment

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

I think possibly this needs to stay integer as it's part of the schema, and we should put anyof or oneOf in the possible responses

response:
  status:
    oneOf:
      - 200
      - 202

Copy link
Member Author

Choose a reason for hiding this comment

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

The return doesn't matches with the elements of array. it has content type too but that we have already passed in the schema.

ERROR   flow_framework/workflow/provision.yaml (tests/default/flow_framework/workflow/provision.yaml)
    ERROR   CHAPTERS
        ERROR   Provision workflow.
            PASSED  PARAMETERS
                PASSED  workflow_id
            PASSED  REQUEST BODY
            ERROR   RESPONSE STATUS (Expected status 200,202, but received 200: application/json.)
            SKIPPED RESPONSE PAYLOAD BODY
            SKIPPED RESPONSE PAYLOAD SCHEMA
            SKIPPED RESPONSE OUTPUT VALUES

Copy link
Collaborator

@nhtruong nhtruong Mar 21, 2025

Choose a reason for hiding this comment

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

response:
  status:
    oneOf:
      - 200
      - 202

I dont think this is allowed in JSON Schema. Each element of oneOf must be a valid JSON Schema, which 200 and 202 are not. Also we're not describing a schema; this is the data itself. So we shouldn't use JSON schema terminologies here.

The prologues and epilogues can already handle an array of status codes. Now that main chapters can handle arrays too, I think we should DRY the status logic and schema so that the expected status for all chapters can be either an array or an integer.

@owaiskazi19 owaiskazi19 force-pushed the fix-error-code branch 8 times, most recently from 329caa9 to d566c1f Compare March 20, 2025 21:25
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.

4 participants