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

Provide async APIs for CsdlWriter and SchemaWriter #3006

Merged
merged 25 commits into from
Jul 23, 2024

Conversation

WanjohiSammy
Copy link
Contributor

@WanjohiSammy WanjohiSammy commented Jun 20, 2024

Issues

This pull request fixes #2684

Description

The current CsdlWriter class provides only synchronous methods for writing CSDL (WriteCsdl and TryWriteCsdl).
Also SchemaWriter class exposes only syncronous methods for writing Schema (TryWriteSchema)

Main Changes

1. Provide APIs for writing CSDL asynchronously:

  • WriteCsdlAsync
  • TryWriteCsdlAsync
  • TryWriteSchemaAsync

To add these async APIs methods necessitates updates to the following files to support asynchronous operations:

- src/Microsoft.OData.Edm/Csdl/CsdlJsonWriter.cs
- src/Microsoft.OData.Edm/Csdl/CsdlWriter.cs
- src/Microsoft.OData.Edm/Csdl/CsdlXmlWriter.cs
- src/Microsoft.OData.Edm/Csdl/SchemaWriter.cs
- src/Microsoft.OData.Edm/Csdl/Serialization/EdmModelCsdlSchemaJsonWriter.cs
- src/Microsoft.OData.Edm/Csdl/Serialization/EdmModelCsdlSchemaWriter.cs
- src/Microsoft.OData.Edm/Csdl/Serialization/EdmModelCsdlSchemaXmlWriter.cs
- src/Microsoft.OData.Edm/Csdl/Serialization/EdmModelCsdlSerializationVisitor.cs
- src/Microsoft.OData.Edm/Csdl/Serialization/EdmModelReferenceElementsJsonVisitor.cs
- src/Microsoft.OData.Edm/Csdl/Serialization/EdmModelReferenceElementsXmlVisitor.cs
- src/Microsoft.OData.Edm/EdmModelVisitor.cs

These new asynchronous APIs have been documented in PublicAPI.Unshipped.txt.

2. Writing tests for the new asynchronous CsdlWriter APIs:

Adapting the existing tests to utilize the async/await pattern for the newly introduced asynchronous methods in the:

- test/FunctionalTests/Microsoft.OData.Core.Tests/ScenarioTests/Roundtrip/ContextUrlWriterReaderTests.cs
- test/FunctionalTests/Microsoft.OData.Edm.Tests/Csdl/CsdlReaderTests.cs
- test/FunctionalTests/Microsoft.OData.Edm.Tests/Csdl/CsdlWriterTests.TargetPath.cs
- test/FunctionalTests/Microsoft.OData.Edm.Tests/Csdl/CsdlWriterTests.VocabularyAnnotation.cs
- test/FunctionalTests/Microsoft.OData.Edm.Tests/Csdl/CsdlWriterTests.cs
- test/FunctionalTests/Microsoft.OData.Edm.Tests/Csdl/Serialization/EdmModelCsdlSchemaWriterTests.cs
- test/FunctionalTests/Microsoft.OData.Edm.Tests/Csdl/Serialization/EdmModelCsdlSerializationVisitorTests.cs
- test/FunctionalTests/Microsoft.OData.Edm.Tests/ScenarioTests/OasisActionsFunctionsRelationshipChangesAcceptanceTests.cs
- test/FunctionalTests/Microsoft.OData.Edm.Tests/ScenarioTests/OasisRelationshipChangesAcceptanceTests.cs
- test/FunctionalTests/Microsoft.OData.Edm.Tests/Vocabularies/AlternateKeysVocabularyTests.cs
- test/FunctionalTests/Microsoft.OData.Edm.Tests/Vocabularies/AuthorizationVocabularyTests.cs
- test/FunctionalTests/Microsoft.OData.Edm.Tests/Vocabularies/CapabilitiesVocabularyTests.cs
- test/FunctionalTests/Microsoft.OData.Edm.Tests/Vocabularies/CommunityVocabularyTests.cs
- test/FunctionalTests/Microsoft.OData.Edm.Tests/Vocabularies/CoreVocabularyTests.cs
- test/FunctionalTests/Microsoft.OData.Edm.Tests/Vocabularies/ValidationVocabularyTests.cs

Checklist (Uncheck if it is not completed)

  • Test cases added
  • Build and test with one-click build and test script passed

@WanjohiSammy WanjohiSammy marked this pull request as ready for review June 20, 2024 12:05
habbes
habbes previously approved these changes Jul 3, 2024
@ElizabethOkerio
Copy link
Contributor

ElizabethOkerio commented Jul 9, 2024

If you do $metadata on your endpoint will the metadata document be written asynchronously or that is not yet supported? If not, do we plan on supporting that?

@WanjohiSammy
Copy link
Contributor Author

WanjohiSammy commented Jul 9, 2024

If you do $metadata on your endpoint will the metadata document be written asynchronously or that is not yet supported? If not, do we plan on supporting that?

@ElizabethOkerio

Thanks for bringing this up. However, this might be a breaking change since it will involve modify the WriteMetadataDocumentAsync implementation in both ODataMetadataJsonOutputContext and ODataMetadataOutputContext

In both:

Change From

internal override Task WriteMetadataDocumentAsync()
{
    this.AssertAsynchronous();

    return TaskUtils.GetTaskForSynchronousOperationReturningTask(
        () =>
        {
            this.WriteMetadataDocumentImplementation();
            return this.FlushAsync();
        });
}

to

internal override async Task WriteMetadataDocumentAsync()
{
    this.AssertAsynchronous();

    await this.WriteMetadataDocumentImplementationAsync().ConfigureAwait(false);
    await this.FlushAsync().ConfigureAwait(false);
}

@WanjohiSammy
Copy link
Contributor Author

WanjohiSammy commented Jul 9, 2024

If you do $metadata on your endpoint will the metadata document be written asynchronously or that is not yet supported? If not, do we plan on supporting that?

@ElizabethOkerio

Thanks for bringing this up. However, this might be a breaking change since it will involve modify the WriteMetadataDocumentAsync implementation in both ODataMetadataJsonOutputContext and ODataMetadataOutputContext

@ElizabethOkerio

Should I go forward and change these implementations in this PR or can be done in a different PR.
And how will this change reflects on WebAPI

@gathogojr
Copy link
Contributor

If you do $metadata on your endpoint will the metadata document be written asynchronously or that is not yet supported? If not, do we plan on supporting that?

@ElizabethOkerio

Thanks for bringing this up. However, this might be a breaking change since it will involve modify the WriteMetadataDocumentAsync implementation in both ODataMetadataJsonOutputContext and ODataMetadataOutputContext

In both:

Change From

internal override Task WriteMetadataDocumentAsync()
{
    this.AssertAsynchronous();

    return TaskUtils.GetTaskForSynchronousOperationReturningTask(
        () =>
        {
            this.WriteMetadataDocumentImplementation();
            return this.FlushAsync();
        });
}

to

internal override async Task WriteMetadataDocumentAsync()
{
    this.AssertAsynchronous();

    await this.WriteMetadataDocumentImplementationAsync().ConfigureAwait(false);
    await this.FlushAsync().ConfigureAwait(false);
}

It wouldn't be a breaking change if it doesn't change what's written on the wire...

In addition, part of the objective for implementing the async APIs was so we could remove these async wrapper over sync APIs. The change you mentioned above is the right/desired change. Make sure to add tests to verify that the output is the same from both WriteMetadataDocument and WriteMetadataDocumentAsync

@WanjohiSammy
Copy link
Contributor Author

WanjohiSammy commented Jul 10, 2024

It wouldn't be a breaking change if it doesn't change what's written on the wire...

In addition, part of the objective for implementing the async APIs was so we could remove these async wrapper over sync APIs. The change you mentioned above is the right/desired change. Make sure to add tests to verify that the output is the same from both WriteMetadataDocument and WriteMetadataDocumentAsync

@gathogojr

This has been done and pushed in this commit: 64977a6

I have added tests to compare both XML and JSON metadata generated by WriteMetadataDocument and WriteMetadataDocumentAsync

xuzhg
xuzhg previously approved these changes Jul 12, 2024
…ceTests to OasisActionsFunctionsRelationshipChangesAcceptanceTest to reuse the methods used in main class OasisActionsFunctionsRelationshipChangesAcceptanceTest
@WanjohiSammy WanjohiSammy merged commit b94324c into dev-8.x Jul 23, 2024
5 checks passed
WanjohiSammy added a commit that referenced this pull request Sep 6, 2024
WanjohiSammy added a commit that referenced this pull request Sep 9, 2024
* Add corresponding async methods for syncronous methods of EdmModelCsdlSchemaJsonWriter

* Implemented asynchronous counterparts for synchronous virtual and abstract methods for EdmModelCsdlSchemaWriter class

* Added asynchronous implementations for the synchronous methods in EdmModelCsdlSchemaXmlWriter

* Added corresponding asynchronous methods for EdmModelReferenceElementsJsonVisitor, EdmModelCsdlSerializationVisitor and EdmModelReferenceElementsXmlVisitor

* Added asynchronous counterparts for csdl writer methods

* Added async corresponding sync methods for EdmModelVisitor

* Fix WriteStartElementAsync prefix and namespace missing

* Added csdl async/await and functional tests for csdl writer async methods

* Added async APIs to net48 public API

* Fixed comments to add ConfigureAwait(false)

* Added configureAwait(false) to await tests

* Elude await/async in BeginElementAsync method Func<TElement, Task> params

* Rewrite the doc string of the async methods to add <returns> and provide correct summary

* Rewrite WriteSchemata to WriteSchema

* Move the asynhronous tests to .Async partial classes

* Added ContextUrlWriterReaderTests.Async partial class with async tests and fix asynhronous equal true when writing xml

* Added doc string for <params></params>

* Resolve missing spaces

* Added Returns Documentation string for WriteCsdlAsync

* Remove unnecessary 'else'

* Added WriteMetadataDocumentAsync methods and tests

* Rename test functions for WriteMetadataDocumentAsync

* Renaming WriteSchema to WriteSchemas and WriteSchemaAsync to WriteSchemasAsync

* Provide the correct file in copyright for the added files

* Rename partial class OasisActionsFunctionsRelationshipChangesAcceptanceTests to OasisActionsFunctionsRelationshipChangesAcceptanceTest to reuse the methods used in main class OasisActionsFunctionsRelationshipChangesAcceptanceTest
WanjohiSammy added a commit that referenced this pull request Sep 30, 2024
…r and SchemaWriter (#3006) (#3060)

* Provide async APIs for CsdlWriter and SchemaWriter (#3006)
@WanjohiSammy WanjohiSammy deleted the fix/2684-csdlwriter-async-apis branch November 13, 2024 13:20
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.

6 participants