azure-rest-api-specs icon indicating copy to clipboard operation
azure-rest-api-specs copied to clipboard

Fix recursive reference

Open evaou opened this issue 1 year ago • 5 comments

Data Plane API - Pull Request

The change is to fix recursive reference for "ErrorDetail" definition by introducing "SubErrorDetail" definition to limit the reference to one level down only.

API Info: The Basics

Most of the information about your service should be captured in the issue that serves as your API Spec engagement record.

  • Link to API Spec engagement record issue:

Is this review for (select one):

  • [ ] a private preview
  • [ ] a public preview
  • [ ] GA release

Change Scope

This section will help us focus on the specific parts of your API that are new or have been modified.
Please share a link to the design document for the new APIs, a link to the previous API Spec document (if applicable), and the root paths that have been updated.

  • Design Document:
  • Previous API Spec Doc:
  • Updated paths:

❔Got questions? Need additional info?? We are here to help!

Contact us!

The Azure API Review Board is dedicated to helping you create amazing APIs. You can read about our mission and learn more about our process on our wiki.

Click here for links to tools, specs, guidelines & other good stuff

Tooling

Guidelines & Specifications

Helpful Links

evaou avatar Jul 14 '23 08:07 evaou

Hi, @evaou! Thank you for your pull request. To help get your PR merged:

  • Ensure you reviewed the checklists in the PR description.
  • Know that PR assignee is the person auto-assigned and responsible for your current PR review and approval.
  • For convenient view of the API changes made by this PR, refer to the URLs provided in the table in the Generated ApiView comment added to this PR. You can use ApiView to show API versions diff.
  • Swagger Validation Report

    ️❌BreakingChange: 2 Errors, 0 Warnings failed [Detail]
    compared swaggers (via Oad v0.10.4)] new version base version
    types.json 1.0(0ff11b3) 1.0(main)
    Rule Message
    1017 - ReferenceRedirection The '$ref' property points to different models in the old and new versions.
    New: common-types/data-plane/v1/types.json#L61:11
    Old: common-types/data-plane/v1/types.json#L31:11
    1033 - RemovedProperty The new version is missing a property found in the old version. Was 'details' renamed or removed?
    New: common-types/data-plane/v1/types.json#L12:7
    Old: common-types/data-plane/v1/types.json#L12:7
    ️️✔️Breaking Change(Cross-Version) succeeded [Detail] [Expand]
    There are no breaking changes.
    ️️✔️CredScan succeeded [Detail] [Expand]
    There is no credential detected.
    ️️✔️LintDiff succeeded [Detail] [Expand]
    Validation passes for LintDiff.
    ️️✔️Avocado succeeded [Detail] [Expand]
    Validation passes for Avocado.
    ️️✔️ApiReadinessCheck succeeded [Detail] [Expand]
    ️️✔️~[Staging] ServiceAPIReadinessTest succeeded [Detail] [Expand]
    Validation passes for ServiceAPIReadinessTest.
    ️❌SwaggerAPIView: 1 Errors, 0 Warnings failed [Detail]
    Rule Message
    Failed to generate swagger APIView. The readme file format is invalid and the tag is not defined. Use the provided readme template for guidance readme template. For more details, please check the detail log. "How to fix":"Check the readme file and make sure the readme file format is valid and the tag is defined. Use the provided readme template"
    ️️✔️CadlAPIView succeeded [Detail] [Expand]
    ️️✔️TypeSpecAPIView succeeded [Detail] [Expand]
    ️️✔️ModelValidation succeeded [Detail] [Expand]
    Validation passes for ModelValidation.
    ️️✔️SemanticValidation succeeded [Detail] [Expand]
    Validation passes for SemanticValidation.
    ️️✔️PoliCheck succeeded [Detail] [Expand]
    Validation passed for PoliCheck.
    ️️✔️PrettierCheck succeeded [Detail] [Expand]
    Validation passes for PrettierCheck.
    ️️✔️SpellCheck succeeded [Detail] [Expand]
    Validation passes for SpellCheck.
    ️️✔️Lint(RPaaS) succeeded [Detail] [Expand]
    Validation passes for Lint(RPaaS).
    ️️✔️CadlValidation succeeded [Detail] [Expand]
    Validation passes for CadlValidation.
    ️️✔️TypeSpec Validation succeeded [Detail] [Expand]
    Validation passes for TypeSpec Validation.
    ️️✔️PR Summary succeeded [Detail] [Expand]
    Validation passes for Summary.
    ️⌛Please ignore, experimental check pending [Detail]
    Posted by Swagger Pipeline | How to fix these errors?

    Swagger Generation Artifacts

    ️️✔️ApiDocPreview succeeded [Detail] [Expand]
     Please click here to preview with your @microsoft account. 
    ️️✔️SDK Breaking Change Tracking succeeded [Detail] [Expand]

    Breaking Changes Tracking

    Posted by Swagger Pipeline | How to fix these errors?

    Swagger pipeline started successfully. If there is ApiView generated, it will be updated in this comment.

    Hi @evaou! The automation detected breaking changes in this pull request. As a result, it added the BreakingChangeReviewRequired label.

    You cannot proceed with merging this PR until you complete one of the following action items:

    ACTION ITEM ALTERNATIVE A: Fix the breaking change.
    Please consult the documentation provided in the relevant validation failures.

    ACTION ITEM ALTERNATIVE B: Request approval.
    Alternatively, if you cannot fix the breaking changes, then you can request an approval for them. Please follow the process described in the High-level Breaking Change Process doc.

    ACTION ITEM ALTERNATIVE C: Report false positive.
    If you think there are no breaking changes, i.e. the validation should pass yet it fails, then proceed as explained in ACTION ITEM ALTERNATIVE B.
    This applies even if the breaking change tool fails with internal runtime error. In such case a manual breaking change review is necessary.

    @weidongxu-microsoft it's under common-types but only used by Email and our Maps services. The file was created by our members previously but have the wrong recursive reference of details field so some 3rd party parsing library like json-schema-ref-parser will detect error and having exception. We are trying the way to fix it without adding new fields too keep generated structure the same. Or do you suggest to have a data-plane/v2 version for it?

    winest avatar Jul 18 '23 03:07 winest

    @winest The common-types swagger should be in sync with the guideline https://github.com/microsoft/api-guidelines/blob/vNext/azure/Guidelines.md#rest-error-response-body-structure

    I think the guideline still uses the recursive reference (ErrorDetail).

    There were lots of existing recursive reference or potential recursive in swagger (and lots of 3rd party lib do work with recursive). I don't think a shortcoming (or bug) in one 3rd party lib would reversely impact whether a Swagger is valid or not.

    If you really need to change this file, we will need to loop in Mike.

    One alternative I can think of, is you use a e.g. common.json in your RP folder, not this shared one. But may still worth a check with Mike before you undergo it.

    weidongxu-microsoft avatar Jul 18 '23 03:07 weidongxu-microsoft

    I try to update the version of json-schema-ref-parser to the latest one and still hit the recursive reference case. I'll email both Mike and Jeffrey for furhter guidance.

    evaou avatar Jul 18 '23 07:07 evaou

    Hi, @evaou. Your PR has no update for 14 days and it is marked as stale PR. If no further update for over 14 days, the bot will close the PR. If you want to refresh the PR, please remove no-recent-activity label.

    Hi, @evaou. The PR will be closed since the PR has no update for 28 days. If you still need the PR review to proceed, please reopen it and @ mention PR assignee.

    Hi, @evaou. The PR will be closed since the PR has no update for 28 days. If you still need the PR review to proceed, please reopen it and @ mention PR assignee.

    Hi, @evaou. The PR will be closed since the PR has no update for 28 days. If you still need the PR review to proceed, please reopen it and @ mention PR assignee.

    Hi, @evaou. The PR will be closed since the PR has no update for 28 days. If you still need the PR review to proceed, please reopen it and @ mention PR assignee.

    Hi, @evaou. The PR will be closed since the PR has no update for 28 days. If you still need the PR review to proceed, please reopen it and @ mention PR assignee.

    Hi, @evaou. The PR will be closed since the PR has no update for 28 days. If you still need the PR review to proceed, please reopen it and @ mention PR assignee.

    Hi, @evaou. The PR will be closed since the PR has no update for 28 days. If you still need the PR review to proceed, please reopen it and @ mention PR assignee.

    Hi, @evaou. The PR will be closed since the PR has no update for 28 days. If you still need the PR review to proceed, please reopen it and @ mention PR assignee.

    Hi, @evaou. The PR will be closed since the PR has no update for 28 days. If you still need the PR review to proceed, please reopen it and @ mention PR assignee.

    Hi, @evaou. The PR will be closed since the PR has no update for 28 days. If you still need the PR review to proceed, please reopen it and @ mention PR assignee.

    Hi, @evaou. The PR will be closed since the PR has no update for 28 days. If you still need the PR review to proceed, please reopen it and @ mention PR assignee.

    Hi, @evaou. The PR will be closed since the PR has no update for 28 days. If you still need the PR review to proceed, please reopen it and @ mention PR assignee.

    Hi, @evaou. The PR will be closed since the PR has no update for 28 days. If you still need the PR review to proceed, please reopen it and @ mention PR assignee.

    Hi, @evaou. The PR will be closed since the PR has no update for 28 days. If you still need the PR review to proceed, please reopen it and @ mention PR assignee.

    Hi, @evaou. The PR will be closed since the PR has no update for 28 days. If you still need the PR review to proceed, please reopen it and @ mention PR assignee.

    Hi, @evaou. The PR will be closed since the PR has no update for 28 days. If you still need the PR review to proceed, please reopen it and @ mention PR assignee.

    Hi, @evaou. The PR will be closed since the PR has no update for 28 days. If you still need the PR review to proceed, please reopen it and @ mention PR assignee.

    Hi, @evaou. The PR will be closed since the PR has no update for 28 days. If you still need the PR review to proceed, please reopen it and @ mention PR assignee.

    Hi, @evaou. The PR will be closed since the PR has no update for 28 days. If you still need the PR review to proceed, please reopen it and @ mention PR assignee.

    Hi, @evaou. The PR will be closed since the PR has no update for 28 days. If you still need the PR review to proceed, please reopen it and @ mention PR assignee.

    Hi, @evaou. The PR will be closed since the PR has no update for 28 days. If you still need the PR review to proceed, please reopen it and @ mention PR assignee.