Skip to content

Conversation

@v-hongli1
Copy link
Member

  1. Fix issue [BUG] create Subscription Rule with CorrelationFilter does not create respect properties  #21299
  2. Changed file:
    1. sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/implementation/ServiceBusManagementSerializer.java
    2. sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/implementation/models/CorrelationFilterImpl.java

@jongio, @YijunXieMS for notification

@ghost ghost added Service Bus customer-reported Issues that are reported by GitHub users external to the Azure organization. labels Jun 25, 2021
@ghost
Copy link

ghost commented Jun 25, 2021

Thank you for your contribution hongli750210! We will review the pull request and get back to you soon.

@v-hongli1
Copy link
Member Author

v-hongli1 commented Jun 25, 2021

All test cases in [ServiceBusSessionManagerTest] can be successfully executed in environments such as [Test ubuntu2004_111_TestFromSource_surefiretest], so execute the test again in [Test ubuntu2004_111_surefiretest].

@v-hongli1 v-hongli1 marked this pull request as ready for review June 25, 2021 06:10

private static final class PropertiesWrapper {
@JacksonXmlProperty(localName = "KeyValueOfstringanyType")
@JacksonXmlProperty(localName = "KeyValueOfstringanyType", namespace = "http://schemas.microsoft.com/netservices/2010/10/servicebus/connect")
Copy link
Contributor

Choose a reason for hiding this comment

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

This code is generated so we better not change it manually.
@jianghaolu, @srnagar, @alzimmermsft does the code gen customization support this customization?

Copy link
Member

Choose a reason for hiding this comment

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

This should be do-able using a Swagger transform to add a namespace sub-property to the XML declaration of keyValueOfstringanyType.

Copy link
Member

Choose a reason for hiding this comment

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

I agree, this should not be hand-edited. Also, updating the swagger might be useful if all other languages are also doing something similar.

Copy link
Member Author

Choose a reason for hiding this comment

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

This setting is added to correspond to the problem that the namespace attribute of the KeyValueOfstringanyType in the XML is a blank value. If this problem can be corrected through swagger, please inform the location of the swagger configuration file storage.

Copy link
Contributor

@YijunXieMS YijunXieMS Jun 29, 2021

Choose a reason for hiding this comment

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

@alzimmermsft The swagger already has namespace property. But the generated code doesn't have it.
https://github.com/Azure/azure-rest-api-specs/blob/39ffff91d145f6c8f7ec1a6ae46bda17256e8d2e/specification/servicebus/data-plane/servicebus-swagger.json#L1513

    "KeyValue": {
      "description": "Key Values of custom properties",
      "type": "object",
      "xml": {
        "name": "KeyValueOfstringanyType",
        "namespace": "http://schemas.microsoft.com/netservices/2010/10/servicebus/connect"
      },
      "properties": {
        "key": {
          "type": "string",
          "xml": {
            "name": "Key",
            "namespace": "http://schemas.microsoft.com/netservices/2010/10/servicebus/connect"
          }
        },
        "value": {
          "type": "string",
          "xml": {
            "name": "Value",
            "namespace": "http://schemas.microsoft.com/netservices/2010/10/servicebus/connect"
          }
        }
      }
    },

Copy link
Member

Choose a reason for hiding this comment

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

@srnagar, @jianghaolu, @weidongxu-microsoft, could this be an code generation issue when there is a combination of XML wrapped and namespace attribute, where the namespace attribute doesn't get propagated to the wrapped property?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, this is a codegen issue. It's missing the namespace when generating the internal wrapper classes. I'll have this fixed.

Copy link
Member

Choose a reason for hiding this comment

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

PR to fix this - Azure/autorest.java#1081

…lationFilter does not create respect properties 20210628 by lihong
Copy link
Contributor

@YijunXieMS YijunXieMS left a comment

Choose a reason for hiding this comment

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

Let's manually edit the namespace for now and investigate why generated code doesn't have the namespace later.

…lationFilter does not create respect properties 2021063010220 by lihong
@v-hongli1 v-hongli1 requested a review from ki1729 as a code owner June 30, 2021 02:21
@v-hongli1
Copy link
Member Author

@YijunXieMS
namespace property of KeyValueOfstringanyType has been added in SqlFilter.java and SqlRuleActionImpl.java.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

customer-reported Issues that are reported by GitHub users external to the Azure organization. Service Bus

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants