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

[Python] Support more testing model #2541

Merged
merged 24 commits into from
Apr 13, 2023
Merged

[Python] Support more testing model #2541

merged 24 commits into from
Apr 13, 2023

Conversation

crisidev
Copy link
Contributor

@crisidev crisidev commented Apr 4, 2023

Motivation and Context

Python should be tested and compliant with all the testing models used in Rust. To achieve this, there are several changes required in terms of customizations, constraint traits and general bug fixing for edge cases.

In languages that are not Rust, we swap non primitive symbols like DateTime, Blob, Document, etc with wrappers that are compatible with the target language.

This was done using a hack that was looking at the original symbol. If that differed from the new one, we called from() or into() to make rustc happy.

Description

This PR implements several changes:

Symbol swapping

In languages that are not Rust, we swap non primitive symbols like DateTime, Blob, Document, etc with wrappers that are compatible with the target language.

During deserialization the runtime parser requires the Rust symbols, while we are handed back the Python wrappers. Customizations are added call .map($type::from) for Option<T>, .into() for T, etc. The customizations are added to the JsonParserGenerator, in the ServerHttpBoundProtocolGenerator and in HttpBindingGenerator.

This should close #2479

Recursive shapes

Implements FromPyObject and IntoPy traits for Box<T> to support recursive shapes. This should close #2476

Naming obstacle

Ensure we use proper casing and escapes for reserved symbol in all Python code (see also #2552)

Constrained Blob

Rust Blobs can be constrained so we need to implement the From trait to allow conversion with the Python Blob type.

This should close #2551

Generate diff for all models

We now also generate diffs for all tested models. This should close #2483

Bug fixing

This PR also fixes a bug in the PythonServerUnionGenerator where we are missing a closing ) in a case of union Unit.


By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

david-perez and others added 2 commits April 4, 2023 09:58
…DataSerializer`

No implementation of the `Protocol` interface makes use of the
`OperationShape` parameter in the `structuredDataParser` and
`structuredDataSerializer` methods.
customizations for JsonParserGenerator and ServerHttpBoundProtocolGenerator.

Signed-off-by: Bigo <1781140+crisidev@users.noreply.github.com>
@crisidev crisidev requested review from a team as code owners April 4, 2023 15:41
@crisidev crisidev marked this pull request as draft April 4, 2023 15:41
@crisidev crisidev added enhancement New feature or request python-server Python server SDK labels Apr 4, 2023
@crisidev crisidev changed the title Remove the TypeConversionGenerator class in favor of using customizations for JsonParserGenerator and ServerHttpBoundProtocolGenerator [WRemove the TypeConversionGenerator class in favor of using customizations for JsonParserGenerator and ServerHttpBoundProtocolGenerator Apr 4, 2023
@crisidev crisidev changed the title [WRemove the TypeConversionGenerator class in favor of using customizations for JsonParserGenerator and ServerHttpBoundProtocolGenerator [WIP][Python] Remove the TypeConversionGenerator class in favor of using customizations Apr 4, 2023
@github-actions
Copy link

github-actions bot commented Apr 4, 2023

A new generated diff is ready to view.

  • No codegen difference in the AWS SDK
  • No codegen difference in the Client Test
  • Server Test (ignoring whitespace)
  • No codegen difference in the Server Test Python

A new doc preview is ready to view.

@crisidev
Copy link
Contributor Author

crisidev commented Apr 4, 2023

I need to fix a bunch of unit tests, but the code is almost ready.

@github-actions
Copy link

github-actions bot commented Apr 4, 2023

A new generated diff is ready to view.

  • No codegen difference in the AWS SDK
  • No codegen difference in the Client Test
  • Server Test (ignoring whitespace)
  • No codegen difference in the Server Test Python

A new doc preview is ready to view.

@github-actions
Copy link

github-actions bot commented Apr 4, 2023

A new generated diff is ready to view.

  • No codegen difference in the AWS SDK
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python

A new doc preview is ready to view.

@crisidev
Copy link
Contributor Author

crisidev commented Apr 5, 2023

Is the PR diff bot still broken? I would expect changes in Python..

@github-actions
Copy link

github-actions bot commented Apr 5, 2023

A new generated diff is ready to view.

  • No codegen difference in the AWS SDK
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python

A new doc preview is ready to view.

@crisidev crisidev changed the title [WIP][Python] Remove the TypeConversionGenerator class in favor of using customizations [Python] Remove the TypeConversionGenerator class in favor of using customizations Apr 5, 2023
@crisidev crisidev marked this pull request as ready for review April 5, 2023 11:26
@github-actions
Copy link

github-actions bot commented Apr 5, 2023

A new generated diff is ready to view.

  • No codegen difference in the AWS SDK
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python

A new doc preview is ready to view.

@github-actions
Copy link

github-actions bot commented Apr 8, 2023

A new generated diff is ready to view.

  • No codegen difference in the AWS SDK
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python
  • No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

@github-actions
Copy link

A new generated diff is ready to view.

  • No codegen difference in the AWS SDK
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python
  • No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

@github-actions
Copy link

A new generated diff is ready to view.

  • No codegen difference in the AWS SDK
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python
  • No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

@github-actions
Copy link

A new generated diff is ready to view.

  • No codegen difference in the AWS SDK
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python
  • No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

class ServerRestJsonFactory : ProtocolGeneratorFactory<ServerHttpBoundProtocolGenerator, ServerCodegenContext> {
override fun protocol(codegenContext: ServerCodegenContext): Protocol = ServerRestJsonProtocol(codegenContext)
class ServerRestJsonFactory(
private val additionalParserCustomizations: List<JsonParserCustomization> = listOf(),
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: Maybe we should have a ServerCustomizations class to house all these arguments?

@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

Signed-off-by: Bigo <1781140+crisidev@users.noreply.github.com>
@github-actions
Copy link

A new generated diff is ready to view.

  • No codegen difference in the AWS SDK
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • Server Test Python (ignoring whitespace)
  • No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

@github-actions
Copy link

A new generated diff is ready to view.

  • No codegen difference in the AWS SDK
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • Server Test Python (ignoring whitespace)
  • No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

Copy link
Collaborator

@rcoh rcoh left a comment

Choose a reason for hiding this comment

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

codegen-core changes LGTM, small non-blocking comment on diff_lib

Comment on lines +64 to +65
get_cmd_output(f"mv {target}/python/build/smithyprojections/{target}-python {OUTPUT_PATH}/")
get_cmd_output(f"mv {target}/typescript/build/smithyprojections/{target}-typescript {OUTPUT_PATH}/")
Copy link
Collaborator

Choose a reason for hiding this comment

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

should this be target == target_codegen_typescript/python instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At the end it does the same.. I have some other rearchitectures of the diff lib in the next PR. I'll refactor this part in it as well if that's ok?

@crisidev crisidev added this pull request to the merge queue Apr 13, 2023
Merged via the queue into main with commit d97defb Apr 13, 2023
@crisidev crisidev deleted the oxipy-testing-models branch April 13, 2023 14:22
Velfi pushed a commit that referenced this pull request Apr 13, 2023
* Remove parameter from `Protocol`s `structuredDataParser`, `structuredDataSerializer`

No implementation of the `Protocol` interface makes use of the
`OperationShape` parameter in the `structuredDataParser` and
`structuredDataSerializer` methods.

* Remove the TypeConversionGenerator class in favor of using
customizations for JsonParserGenerator and ServerHttpBoundProtocolGenerator.

Signed-off-by: Bigo <1781140+crisidev@users.noreply.github.com>

* Make the additionaParserCustomizations default to empty list

* Fix merge conflict

* Fix missing ;

* Use better defaults when checking for customizations

* Use better defaults when checking for customizations

* Add HttpBindingCustomization and relax the datetime symbol check

* Support recursive shapes and add a lot more models to the tests

Signed-off-by: Bigo <1781140+crisidev@users.noreply.github.com>

* Support naming obstacle course

* Add support for constrained blobs conversions

* Support constraint traits

* Try to generate the full diff

Signed-off-by: Bigo <1781140+crisidev@users.noreply.github.com>

* A better way of checking if we need to go into the Timestamp branch

* Remove wheels folder

---------

Signed-off-by: Bigo <1781140+crisidev@users.noreply.github.com>
Co-authored-by: david-perez <d@vidp.dev>
Velfi pushed a commit that referenced this pull request Apr 17, 2023
* Remove parameter from `Protocol`s `structuredDataParser`, `structuredDataSerializer`

No implementation of the `Protocol` interface makes use of the
`OperationShape` parameter in the `structuredDataParser` and
`structuredDataSerializer` methods.

* Remove the TypeConversionGenerator class in favor of using
customizations for JsonParserGenerator and ServerHttpBoundProtocolGenerator.

Signed-off-by: Bigo <1781140+crisidev@users.noreply.github.com>

* Make the additionaParserCustomizations default to empty list

* Fix merge conflict

* Fix missing ;

* Use better defaults when checking for customizations

* Use better defaults when checking for customizations

* Add HttpBindingCustomization and relax the datetime symbol check

* Support recursive shapes and add a lot more models to the tests

Signed-off-by: Bigo <1781140+crisidev@users.noreply.github.com>

* Support naming obstacle course

* Add support for constrained blobs conversions

* Support constraint traits

* Try to generate the full diff

Signed-off-by: Bigo <1781140+crisidev@users.noreply.github.com>

* A better way of checking if we need to go into the Timestamp branch

* Remove wheels folder

---------

Signed-off-by: Bigo <1781140+crisidev@users.noreply.github.com>
Co-authored-by: david-perez <d@vidp.dev>
unexge pushed a commit that referenced this pull request Apr 24, 2023
* Remove parameter from `Protocol`s `structuredDataParser`, `structuredDataSerializer`

No implementation of the `Protocol` interface makes use of the
`OperationShape` parameter in the `structuredDataParser` and
`structuredDataSerializer` methods.

* Remove the TypeConversionGenerator class in favor of using
customizations for JsonParserGenerator and ServerHttpBoundProtocolGenerator.

Signed-off-by: Bigo <1781140+crisidev@users.noreply.github.com>

* Make the additionaParserCustomizations default to empty list

* Fix merge conflict

* Fix missing ;

* Use better defaults when checking for customizations

* Use better defaults when checking for customizations

* Add HttpBindingCustomization and relax the datetime symbol check

* Support recursive shapes and add a lot more models to the tests

Signed-off-by: Bigo <1781140+crisidev@users.noreply.github.com>

* Support naming obstacle course

* Add support for constrained blobs conversions

* Support constraint traits

* Try to generate the full diff

Signed-off-by: Bigo <1781140+crisidev@users.noreply.github.com>

* A better way of checking if we need to go into the Timestamp branch

* Remove wheels folder

---------

Signed-off-by: Bigo <1781140+crisidev@users.noreply.github.com>
Co-authored-by: david-perez <d@vidp.dev>
rcoh pushed a commit that referenced this pull request Apr 24, 2023
* Remove parameter from `Protocol`s `structuredDataParser`, `structuredDataSerializer`

No implementation of the `Protocol` interface makes use of the
`OperationShape` parameter in the `structuredDataParser` and
`structuredDataSerializer` methods.

* Remove the TypeConversionGenerator class in favor of using
customizations for JsonParserGenerator and ServerHttpBoundProtocolGenerator.

Signed-off-by: Bigo <1781140+crisidev@users.noreply.github.com>

* Make the additionaParserCustomizations default to empty list

* Fix merge conflict

* Fix missing ;

* Use better defaults when checking for customizations

* Use better defaults when checking for customizations

* Add HttpBindingCustomization and relax the datetime symbol check

* Support recursive shapes and add a lot more models to the tests

Signed-off-by: Bigo <1781140+crisidev@users.noreply.github.com>

* Support naming obstacle course

* Add support for constrained blobs conversions

* Support constraint traits

* Try to generate the full diff

Signed-off-by: Bigo <1781140+crisidev@users.noreply.github.com>

* A better way of checking if we need to go into the Timestamp branch

* Remove wheels folder

---------

Signed-off-by: Bigo <1781140+crisidev@users.noreply.github.com>
Co-authored-by: david-perez <d@vidp.dev>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request python-server Python server SDK
Projects
None yet
6 participants