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.

Sign up for GitHub

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

Jump to bottom

Cherry-pick b94324c from 8.x to 7.x: Provide async APIs for CsdlWriter and SchemaWriter (#3006) #3060

Merged
merged 9 commits into from
Sep 30, 2024
Merged

Cherry-pick b94324c from 8.x to 7.x: Provide async APIs for CsdlWriter and SchemaWriter (#3006) #3060

merged 9 commits into from
Sep 30, 2024

Conversation

WanjohiSammy
Copy link
Contributor

Issues

This pull request fixes #xxx.

Description

  • Cherry-pick b94324c from 8.x to 7.x
  • Ensure compatibility with older .NET versions.

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

Provide APIs for writing CSDL asynchronously:

  • WriteCsdlAsync
  • TryWriteCsdlAsync
  • TryWriteSchemaAsync

Checklist (Uncheck if it is not completed)

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

Additional work necessary

If documentation update is needed, please add "Docs Needed" label to the issue and provide details about the required document change in the issue.

@WanjohiSammy
Provide async APIs for CsdlWriter and SchemaWriter (#3006)
d5f88cf 
* 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 WanjohiSammy changed the title Provide async APIs for CsdlWriter and SchemaWriter (#3006) Cherry-pick b94324c from 8.x to 7.x: Provide async APIs for CsdlWriter and SchemaWriter (#3006) Sep 9, 2024
habbes
habbes requested changes Sep 10, 2024
View reviewed changes
Copy link
Contributor

@habbes habbes left a comment

Choose a reason for hiding this comment

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

Seems there's a lot of cases where we use Task.FromResult(0) where Task.CompletedTask appears to be more appropriate, and there are cases missing ConfigureAwait(false).

xuzhg
xuzhg reviewed Sep 19, 2024
View reviewed changes
src/Microsoft.OData.Core/ErrorUtils.cs
/// <param name="error">The error instance to write.</param>
/// <param name="includeDebugInformation">A flag indicating whether error details should be written (in debug mode only) or not.</param>
/// <param name="maxInnerErrorDepth">The maximum number of nested inner errors to allow.</param>
internal static async Task WriteXmlErrorAsync(XmlWriter writer, ODataError error, bool includeDebugInformation, int maxInnerErrorDepth)
Copy link
Member

Choose a reason for hiding this comment

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

Do we still write the XML payload?
I don't think so, we only output the JSON payload exception the CSDL, right?
If that's the case, why do we need to implement the XML related async methods?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are still some async tests and async methods that are calling this method.

xuzhg
xuzhg previously approved these changes Sep 19, 2024
View reviewed changes
habbes
habbes previously approved these changes Sep 23, 2024
View reviewed changes
@WanjohiSammy WanjohiSammy dismissed stale reviews from habbes and xuzhg via 2cdb6c4 September 25, 2024 09:50
habbes
habbes previously approved these changes Sep 25, 2024
View reviewed changes
gathogojr
gathogojr reviewed Sep 27, 2024
View reviewed changes
src/Microsoft.OData.Core/ErrorUtils.cs Outdated Show resolved Hide resolved
src/Microsoft.OData.Core/ErrorUtils.cs Outdated Show resolved Hide resolved
src/Microsoft.OData.Core/ErrorUtils.cs Outdated Show resolved Hide resolved
src/Microsoft.OData.Core/ErrorUtils.cs Outdated Show resolved Hide resolved
src/Microsoft.OData.Core/ErrorUtils.cs Outdated Show resolved Hide resolved
src/Microsoft.OData.Edm/Csdl/Serialization/EdmModelReferenceElementsJsonVisitor.Async.cs Show resolved Hide resolved
src/Microsoft.OData.Edm/Csdl/Serialization/EdmModelReferenceElementsJsonVisitor.Async.cs Show resolved Hide resolved
src/Microsoft.OData.Edm/Csdl/Serialization/EdmModelReferenceElementsJsonVisitor.Async.cs Show resolved Hide resolved
src/Microsoft.OData.Edm/Helpers/TaskUtils.cs Outdated Show resolved Hide resolved
src/Microsoft.OData.Edm/EdmModelVisitor.cs Show resolved Hide resolved
gathogojr
gathogojr previously approved these changes Sep 27, 2024
View reviewed changes
Copy link
Contributor

@gathogojr gathogojr left a comment

Choose a reason for hiding this comment

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

Left a few comments; otherwise LGTM

gathogojr
gathogojr previously approved these changes Sep 27, 2024
View reviewed changes
@WanjohiSammy WanjohiSammy merged commit 61b9bfc into release-7.x Sep 30, 2024
5 checks passed
@WanjohiSammy WanjohiSammy deleted the fix/backport-async-csdwriter-to-7.x branch November 13, 2024 13:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gathogojr gathogojr gathogojr approved these changes

@habbes habbes habbes approved these changes

@xuzhg xuzhg Awaiting requested review from xuzhg

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@WanjohiSammy @habbes @xuzhg @gathogojr