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

Proposal: improve the xml schema to support CDATA(the data does not need to be escaped) #4919

Conversation

TheNorthMemory
Copy link
Contributor

@TheNorthMemory TheNorthMemory commented Oct 3, 2018

Description

  1. implement the xml schema to support CDATA feature, see this
  2. let additionalProperties: {} works same as additionalProperties: true

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Checklist

My PR contains...

  • No code changes (src/ is unmodified: changes to documentation, CI, metadata, etc.)
  • Dependency changes (any modification to dependencies in package.json)
  • Bug fixes (non-breaking change which fixes an issue)
  • Improvements (misc. changes to existing features)
  • Features (non-breaking change which adds functionality)

My changes...

  • are breaking changes to a public API (config options, System API, major UI change, etc).
  • are breaking changes to a private API (Redux, component props, utility functions, etc.).
  • are breaking changes to a developer API (npm script behavior changes, new dev system dependencies, etc).
  • are not breaking changes.

Documentation

  • My changes do not require a change to the project documentation.
  • My changes require a change to the project documentation.
  • If yes to above: I have updated the documentation accordingly.

Automated tests

  • My changes can not or do not need to be tested.
  • My changes can and should be tested by unit and/or integration tests.
  • If yes to above: I have added tests to cover my changes.
  • If yes to above: I have taken care to cover edge cases in my tests.
  • All new and existing tests passed.

TheNorthMemory added a commit to TheNorthMemory/wechat-swagger that referenced this pull request Oct 5, 2018
Copy link
Contributor

@shockey shockey left a comment

Choose a reason for hiding this comment

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

XML Objects are not allowed to have a CDATA field.

If this is changed to use a field named x-cdata instead, I'd be open to merging it 😄

@TheNorthMemory
Copy link
Contributor Author

@shockey I've changed CDATA insteed of x-cdata @ 50a4b4c , do I need pull requests ?

@TheNorthMemory
Copy link
Contributor Author

@shockey the xml cdata feature is ready for merging

@shockey
Copy link
Contributor

shockey commented Oct 23, 2018

thanks @TheNorthMemory, I'm looking at whether this could be better suited as a plugin - stay tuned 😄

@TheNorthMemory
Copy link
Contributor Author

@shockey ops, the xml function was already as a plugin (src/core/plugins/samples/fn.js), does it need another one ?

@TheNorthMemory
Copy link
Contributor Author

the x-cdata feature is scoped @ here, an example is here, hopes may help somebody.

@tim-lai
Copy link
Contributor

tim-lai commented Jun 15, 2020

@TheNorthMemory Following up on this long outstanding PR... There has been subsequent changes to the use of additionalProperties === true, specifically #5072. If this feature is still needed, please update the plugin and I would be happy to re-review. Thanks!

@TheNorthMemory
Copy link
Contributor Author

Yep, the CDATA data is easier readable, let me update this PR soon

@TheNorthMemory
Copy link
Contributor Author

TheNorthMemory commented Jun 16, 2020

@tim-lai merged, on the top of this PR, it supporting the following display now. The characters are safe and easier readable both on xml and json definitions.

<foo><![CDATA[&bar]]></foo>
<bar><![CDATA[<2]]></bar>

@tim-lai tim-lai dismissed shockey’s stale review June 16, 2020 17:45

feedback incorporated

@tim-lai
Copy link
Contributor

tim-lai commented Jun 16, 2020

@TheNorthMemory Thanks for updating! One more request, can you add some notes about usage of this feature to documentation?

@TheNorthMemory
Copy link
Contributor Author

TheNorthMemory commented Jun 17, 2020

This was the src/core/plugins/samples/fn.js implementation. Two no break change. Let me find a place to record those features.

@TheNorthMemory
Copy link
Contributor Author

TheNorthMemory commented Jun 17, 2020

I cannot find a right place to document those two implements. I've also quick check the OAI's #1532 where were discussed about the xml's CDATA. In my opinion, the x-cdata documentation shall be better to place onto OAI project. Because it's about the yaml/json/xml definitions. The x-cdata is only a true/false flag to point out the data does not need to be escaped, the default behavior(false or null) as usual. When setter is on true, means literal. That's it.

The usage examples here:

wechatpay's micropay.request sample:

    scene_info:
      xml:
        x-cdata: true
      type: string
      description: +场景信息
      example: '{"store_info" : {"id": "SZTX001","name": "腾大餐厅","area_code": "440305","address": "科技园中一路腾讯大厦" }}'
      maxLength: 256
      nullable: true

wechatpay's querycombinedorder.response sample:

    - type: object
      xml:
        name: xml
      required:
        - sub_order_list
      properties:
        sub_order_list:
          xml:
            x-cdata: true
          type: string
          description: +子单信息
          example: '{"order_num": 3,"order_list": [{"appid":"wx2421b1c4370ec43b","mch_id":"1230000109","openid":"oUpF8uMuAJO_M2pxb1Q9zNjWeS6o","total_fee":100,"cash_fee":100,"transaction_id":"4217752501201407033233368018","out_trade_no":"12177525012","attach":"test001","time_end":"20171030133525"},{"appid":"wx2421b1c4370ec43b","mch_id":"1230000109","openid":"oUpF8uMuAJO_M2pxb1Q9zNjWeS6o","sub_appid":"wx2421b1c437055ce","sub_mch_id":"1230000108","sub_openid":"oUp3rfMuAJO_123xb1Q9zNjWedco","trade_type":"JSAPI","total_fee":100,"cash_fee":100,"transaction_id":"4217752501201407033233368018","out_trade_no":"12177525012","attach":"test001","time_end":"20171030133525"},{"appid":"wx2421b1c4370ec43b","mch_id":"1230000109","openid":"oUpF8uMuAJO_M2pxb1Q9zNjWeS6o","trade_type":"JSAPI","total_fee":100,"fee_type":"USD","cash_fee":100,"cash_fee_type":"CNY","transaction_id":"4217752501201407033233368018","out_trade_no":"12177525012","attach":"test001","time_end":"20171030133525","rate_value":"65000000"}]}'
          maxLength: 10240

Another one about the additionalProperties: {}, that was already on OAI's 3.0.1 spec. This implement shall more compatible with it.

additionalProperties - Value can be boolean or object. Inline or referenced schema MUST be of a Schema Object and not a standard JSON Schema.

When it's true, should mean anything. While it is defined as plainObject without any properties, it should mean anything too.

Just placed here.

@webron
Copy link
Contributor

webron commented Jun 18, 2020

@TheNorthMemory Apologies for jumping in this late in the game, but I'm afraid we won't be able to accept the PR. While it's great that spec supports extensions, we have to limit our support for custom extensions in the spec. In rare cases, we may choose to add support for such extensions, if we feel that it makes sense for the general Swagger UI users.

Unfortunately, for x-cdata it is not the case. It's just unlikely it'll be helpful to the majority of our users. This is a good example of where plugins come in and you can customize support for such an extension for your own needs. It doesn't mean the work is lost, just that it's not going to be in the core code.

@tim-lai tim-lai self-assigned this Jun 18, 2020
@tim-lai tim-lai closed this Jun 22, 2020
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.

4 participants