Skip to content

fix: change INVALID_INPUT for INVALID_ARGUMENT according to guidelines#129

Merged
akoshunyadi merged 2 commits intocamaraproject:mainfrom
fernandopradocabrillo:fix/align-400-errors
Apr 5, 2024
Merged

fix: change INVALID_INPUT for INVALID_ARGUMENT according to guidelines#129
akoshunyadi merged 2 commits intocamaraproject:mainfrom
fernandopradocabrillo:fix/align-400-errors

Conversation

@fernandopradocabrillo
Copy link
Collaborator

What type of PR is this?

Add one of the following kinds:

  • bug

What this PR does / why we need it:

Change old INVALID_INPUT 400 errors for INVALID_ARGUMENTS as stated in the commonalities guidelines

bigludo7
bigludo7 previously approved these changes Mar 13, 2024
Copy link
Collaborator

@bigludo7 bigludo7 left a comment

Choose a reason for hiding this comment

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

LGTM

value:
status: 400
code: INVALID_ARGUMENT
message: "Expected property is missing: subscriptionId"
Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. If the type is a special error, then the description could be also more concrete.
  2. This special 400 error shouldn't have a Generic400 as example

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not completely getting the special error comment. Since in CAMARA we are using examples to include specific errors, I chose to create a different schema containing both examples. So this way we have two schemas, one containing only Generic 400 for the endpoints that doesn't have subscriptionId and another one containing both.

Is there a better way to do it without adding it inline? or do you mean that it should only have the specific error because the Generic doesn't apply?

What description do you think would be more appropriate?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I thought that since we also have the Generic400 error, then this SubscriptionIdRequired is a specialized one, so it could have a

  • description like "SubscriptionId is missing" and

  • it should only have one example, exactly this case when subscriptionid is missing:

        examples:
              status: 400
              code: INVALID_ARGUMENT
              message: "Expected property is missing: subscriptionId"
    

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As discussed in the meeting we remove the missing suscriptionId example. It would make sense to maintain it if the error code was different, but since it is still INVALID ARGUMENT, we can skip it.

Copy link
Collaborator

@bigludo7 bigludo7 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@akoshunyadi akoshunyadi left a comment

Choose a reason for hiding this comment

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

looks fine

@fernandopradocabrillo
Copy link
Collaborator Author

Hi @akoshunyadi I think we can merge this one so we can include it in the changelog too before the release

@akoshunyadi akoshunyadi merged commit d04d6a4 into camaraproject:main Apr 5, 2024
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.

3 participants