Skip to content

Conversation

apotterri
Copy link
Contributor

@apotterri apotterri commented Mar 10, 2023

Add "properties" and "items" schemas to "parameters" objects , as specified in v1.11.0 of the appmap spec.

Fixes #226 .

@apotterri apotterri force-pushed the param-schema_20230308 branch 2 times, most recently from 1bc1591 to d077fab Compare March 10, 2023 10:58
Copy link
Contributor

@dustinbyrne dustinbyrne left a comment

Choose a reason for hiding this comment

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

This change looks good to me.

Though in my testing, I've been unable to get a return_value from an http_server_response event using Flask 1.1.2. I'm not really sure what the deal is there. I'm not getting any exceptions so I'm inclined to say it's unrelated to this change. Here's the app if you're interested: https://gist.github.com/dustinbyrne/ef0f8094c55d5724ae6f40edbd4a3927

schema_key = "properties"

schema = [_describe_schema(k, v, depth + 1, max_depth) for k, v in elts]
# schema will be [None] if depth is exceeded, don't use it
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks to be the only difference between this implementation and Ruby. In short, an array must always provide a type so in Ruby we'll go beyond the max depth to obtain the typing information for an array. However, we've since added a default which will pass OpenAPI validation in the absence of an array type.

This should work just fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe I ported over all the tests from appmap-ruby. Maybe one showing this behavior is missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a test to check this: getappmap/appmap-ruby#321 .

@apotterri
Copy link
Contributor Author

I'll take a quick look at the Flask issue. Flask 1 got EOLed with the release of Flask 2 on 2021-05-11, though, so I'm not sure how much effort we should devote to supporting it.

@apotterri apotterri requested a review from dustinbyrne March 12, 2023 21:56
Add "properties" and "items" schemas to "parameters" objects , as
specified in v1.11.0 of the appmap spec.
@apotterri apotterri force-pushed the param-schema_20230308 branch from 9977bb5 to 838f2de Compare March 13, 2023 21:49
@apotterri apotterri merged commit a1a5790 into master Mar 13, 2023
@apotterri apotterri deleted the param-schema_20230308 branch March 13, 2023 22:07
@kgilpin
Copy link
Contributor

kgilpin commented Mar 14, 2023

I'll take a quick look at the Flask issue. Flask 1 got EOLed with the release of Flask 2 on 2021-05-11, though, so I'm not sure how much effort we should devote to supporting it.

EOL two years ago? None :-)

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.

Python meets AppMap v1.11.0 specification for updated Parameter object types

3 participants