Skip to content

Conversation

havardf
Copy link
Collaborator

@havardf havardf commented Jun 24, 2025

The idea here is that for some things, as in error handling, its easier to check for compliance by:

  1. check the openapi schema (with new test function test_openapi_schema)
  2. check service against schema (as already done with test_openapi).

@havardf havardf requested a review from ways June 24, 2025 11:30
@havardf havardf linked an issue Jun 24, 2025 that may be closed by this pull request
@ways
Copy link
Collaborator

ways commented Jun 24, 2025

The test seems to work as it should. Found error in edrisobaric schema.

In attempting to fix it I found this: fastapi/fastapi#6799 Not sure if that's enough to fix it, but it seems like a hack.

If this is hard to get right, perhaps it should only be a warning until --strict is used?

)
if not error_response_schema:
errors += (
f"4XX response for {method.upper()} {path} is not valid. Media-type shall be 'application/problem+json'."
Copy link
Collaborator

Choose a reason for hiding this comment

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

If I understand this correctly, it's checking the openapi schema, not an actual response. I don't think that is apparent from the error message.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated the error message. For now, its just checking the media-type. We should maybe check the json response as well, but I think maybe we can add that later. Not sure exactly what to check there.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds good. Perhaps create an issue for doing it some time in the future?

@havardf
Copy link
Collaborator Author

havardf commented Jun 25, 2025

The test seems to work as it should. Found error in edrisobaric schema.

In attempting to fix it I found this: fastapi/fastapi#6799 Not sure if that's enough to fix it, but it seems like a hack.

If this is hard to get right, perhaps it should only be a warning until --strict is used?

Yes, you might be right about that. I will update.

@ways
Copy link
Collaborator

ways commented Jun 25, 2025

What are all the timeout changes? Did you have trouble with timeouts? Don't we have a general --timeout argument for this?

@havardf
Copy link
Collaborator Author

havardf commented Jun 25, 2025

What are all the timeout changes? Did you have trouble with timeouts? Don't we have a general --timeout argument for this?

Had some timeout issues yes, so I just increased the defaults. But yes, all these should use sedr.util.args.timeout. I will update.

@ways ways merged commit b2357de into main Jun 26, 2025
6 checks passed
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.

Check for correct error handling

2 participants