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

Fix JsonSchemaGenerator to handle StructInfo with same alias and name #5788

Merged
merged 1 commit into from
Jun 28, 2024

Conversation

JonathanSawyer
Copy link
Contributor

@JonathanSawyer JonathanSawyer commented Jun 26, 2024

Motivation:

  • The StructInfo alias has the same value as the name causing a duplicate key exception when building typeSignatureToStructMappingBuilder.
  • Occurred when setting up a HTTP service which consumes protobuf messages

Modifications:

Result:

@CLAassistant
Copy link

CLAassistant commented Jun 26, 2024

CLA assistant check
All committers have signed the CLA.

@JonathanSawyer JonathanSawyer changed the title Check that the alias isn't the same as the name. Fix JsonSchemaGenerator to handle aliased protobuf parameters with same value as name Jun 26, 2024
Copy link
Contributor

@nirvanarsc nirvanarsc left a comment

Choose a reason for hiding this comment

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

Nice job 🔧🐸

@JonathanSawyer JonathanSawyer marked this pull request as ready for review June 26, 2024 18:43
@JonathanSawyer JonathanSawyer marked this pull request as draft June 26, 2024 18:44
@JonathanSawyer JonathanSawyer marked this pull request as ready for review June 26, 2024 19:34
@JonathanSawyer JonathanSawyer changed the title Fix JsonSchemaGenerator to handle aliased protobuf parameters with same value as name Fix JsonSchemaGenerator to handle StructInfo with same alias and name Jun 26, 2024
Copy link
Contributor

@jrhee17 jrhee17 left a comment

Choose a reason for hiding this comment

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

👍 👍 👍

@jrhee17 jrhee17 added the defect label Jun 27, 2024
@jrhee17 jrhee17 added this to the 1.30.0 milestone Jun 27, 2024
Copy link
Contributor

@ikhoon ikhoon left a comment

Choose a reason for hiding this comment

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

Nice catch!

@JonathanSawyer
Copy link
Contributor Author

@jrhee17 I was wondering whether this could be added to the 1.29.1 milestone?

@minwoox minwoox modified the milestones: 1.30.0, 1.29.1 Jun 27, 2024
Copy link
Contributor

@minwoox minwoox left a comment

Choose a reason for hiding this comment

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

Thanks!

@minwoox minwoox merged commit cbab1c4 into line:main Jun 28, 2024
15 checks passed
@trustin
Copy link
Member

trustin commented Jun 28, 2024

Sorry I'm late and thanks for the fix, @JonathanSawyer! 🙇

minwoox pushed a commit that referenced this pull request Jun 28, 2024
…#5788)

Motivation:

- The `StructInfo` `alias` has the same value as the `name` causing a duplicate key exception when building  `typeSignatureToStructMappingBuilder`. 
- Occurred when setting up a HTTP service which consumes protobuf messages

Modifications:

- Checks that the `struct` and `alias` don't have the same name to avoid duplicate key exception (alias is created here https://github.com/line/armeria/blob/main/protobuf/src/main/java/com/linecorp/armeria/server/protobuf/ProtobufDescriptiveTypeInfoProvider.java#L106)

Result:

- Improves the `JsonSchemaGenerator.java` for the documentation service by allowing resources to have the same alias and name as is the default behavior of https://github.com/line/armeria/blob/main/protobuf/src/main/java/com/linecorp/armeria/server/protobuf/ProtobufDescriptiveTypeInfoProvider.java#L106

<!--
Visit this URL to learn more about how to write a pull request description:
https://armeria.dev/community/developer-guide#how-to-write-pull-request-description
-->
@JonathanSawyer JonathanSawyer deleted the fix_alias_bug branch June 28, 2024 06:59
@JonathanSawyer
Copy link
Contributor Author

Tested and working 🙇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants