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

adds support for deprecation #155

Merged
merged 17 commits into from
Jan 5, 2022
Merged

adds support for deprecation #155

merged 17 commits into from
Jan 5, 2022

Conversation

baywet
Copy link
Member

@baywet baywet commented Dec 13, 2021

fixes #113

This PR sets deprecated to true on operations that depend on annotables with an annotation of term Org.OData.Core.V1.Revisions and of kind Org.OData.Core.V1.RevisionKind/Deprecated (see the linked issue for more details).

It will also add and extension to the operation to convey the additional information from the annotation like

/People/$count:
    get:
      summary: Get the number of the resource
      operationId: Get.Count.People
      responses:
        '200':
          description: The count of the resource
          content:
            text/plain:
              schema:
                $ref: '#/components/schemas/ODataCountResponse'
        default:
          $ref: '#/components/responses/error'
      deprecated: true
      x-ms-deprecation:
        removalDate: '2023-03-15T00:00:00.0000000'
        date: '2021-08-24T00:00:00.0000000'
        version: 2021-05/people
        description: The People API is deprecated and will stop returning data on March 2023. Please use the new newPeople API.

The flag + extension will be added to any operation if any path segment in the path is annotated including:

  • parent entity sets
  • parent singleton
  • parent navigation properties
  • the type or base type of the nav prop/singleton/entity set

Note: depending on merge order, #149 will need to be updated to return the target type in the annotables for the segment

@baywet baywet self-assigned this Dec 13, 2021
@baywet baywet added this to the 1.0.10 milestone Dec 13, 2021
@baywet baywet marked this pull request as ready for review December 15, 2021 16:19
@baywet baywet enabled auto-merge December 15, 2021 16:20
@xuzhg xuzhg disabled auto-merge December 15, 2021 23:29
using Microsoft.OpenApi.OData.Vocabulary.Core;
using Xunit;

namespace Microsoft.OpenApi.OData.Reader.Vocabulary.Capabilities.Tests;
Copy link
Contributor

Choose a reason for hiding this comment

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

the namespace looks have different pattern comparing to the existing?

Copy link
Member Author

Choose a reason for hiding this comment

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

if it were up to me, I'd have converted all the existing files to this new format, you get more real-estate on the screen. But doing so might generate tons of conflicts since we have multiple people contributing at this time.
So my compromise was to do it only for new files, so they are future proof, and maybe later we can have a PR that updates the existing files once we're done with our batch of modifications.

@@ -24,6 +26,12 @@ public class ODataDollarCountSegment : ODataSegment
public override string Identifier => "$count";

/// <inheritdoc />
public override string GetPathItemName(OpenApiConvertSettings settings, HashSet<string> parameters) => "$count";
public override IEnumerable<IEdmVocabularyAnnotatable> GetAnnotables()
Copy link
Contributor

Choose a reason for hiding this comment

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

alignment the codes, tab -> 4 whitespaces

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems you can implement the default "GetAnnotables()" at the base class to return an empty Enumerable. Only override the method in the class that should be overridden?

Copy link
Member Author

Choose a reason for hiding this comment

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

The reason why I didn't make it virtual is because I want the compiler to error out in case we forgot a segment type instead of silently pass. This is going to come handy as we coordinate with PRs that add other segment types. But it could mean a breaking change for people whom inherited from the OData segment class, so I'm willing to compromise on this one. What do you think?

/// <summary>
/// Provides any deprecation information for the segment.
/// </summary>
public OpenApiDeprecationExtension Deprecation { get; set; }
Copy link
Contributor

Choose a reason for hiding this comment

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

The deprecation looks for the ODataPath, not a certain Segment in the Path, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

each segment can have deprecation information, which means a path can have multiple deprecations. Then the path will take the uppermost (shortest path) deprecation.

Copy link
Member Author

Choose a reason for hiding this comment

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

a good example of that is me/mailFolders/{id}/messages/{id}/attachments where in theory:

  • the user type (or a parent type) could be deprecated
  • the me singleton could be depracated
  • the mailFolder (or a parent type) could be deprecated
  • the mailFolders navigation property could be deprecated
  • ...

Assuming we have deprecation for all of those, the deprecation for the path that will be used, is the one from user, if user is not deprecated, the one from me, if not me, mailfolder, etc....

@baywet baywet force-pushed the feature/deprecated branch from 632b604 to 23c693e Compare December 16, 2021 13:13
@baywet baywet enabled auto-merge December 16, 2021 13:13
@baywet baywet requested a review from xuzhg December 16, 2021 13:14
@baywet baywet force-pushed the feature/deprecated branch from 23c693e to 1aba9ed Compare December 16, 2021 13:35
…e deprecation information

Signed-off-by: Vincent Biret <[email protected]>
- implements adding deprecration information for all the segment types

Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
- updates unit tests following merge

Signed-off-by: Vincent Biret <[email protected]>
@baywet baywet force-pushed the feature/deprecated branch from 1aba9ed to f738e9f Compare December 16, 2021 13:45
@xuzhg
Copy link
Contributor

xuzhg commented Jan 5, 2022

:shipit: Look good to me.

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.

Set deprecated property for operations that have been deprecated
2 participants