-
-
Notifications
You must be signed in to change notification settings - Fork 198
Add obsolete method overloads to restore binary compatibility after CancellationToken changes #641
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
Conversation
…onToken changes Co-authored-by: sebastienros <[email protected]>
@@ -46,6 +46,9 @@ public Task InitializeAsync(IStore store, CancellationToken cancellationToken = | |||
return Task.CompletedTask; | |||
} | |||
|
|||
public Task InitializeAsync(IStore store) | |||
=> InitializeAsync(store, CancellationToken.None); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it may be better to just add the default implementation in the interface instead so we can just remove them from one place in the future because I doubt anyone have a custom implementation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good suggestion! I've moved the obsolete method implementations to the interface as default implementations. This reduces code duplication and makes it easier to remove them in a future release since they'll only need to be removed from one place. The changes are in commit 3ece078.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't suggest this because I am not sure this actually creates the method where it's missing. I tried on a random project to verify. But it might work as I didn't try with the full build, just looked at the generated code and IL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You were absolutely right to be concerned! Default interface methods don't create actual method overloads in the compiled IL of concrete classes, which means binary compatibility would still be broken. I've restored the obsolete method implementations back to the concrete classes and removed the default implementations from interfaces. Now the method overloads exist as actual methods on concrete types like Session
and Store
, ensuring true binary compatibility. Fixed in commit 3d7bb39.
Co-authored-by: MikeAlhayek <[email protected]>
@@ -1170,6 +1170,9 @@ public async Task<int> CountAsync(CancellationToken cancellationToken = default) | |||
} | |||
} | |||
|
|||
public Task<int> CountAsync() | |||
=> CountAsync(CancellationToken.None); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All these method should be removed from every implementation and directly added into the interface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done! I've moved all obsolete method implementations from concrete classes to interface default implementations. The obsolete methods are now only defined once in the interfaces and can be easily removed in a future release. See commit dddbf52.
@@ -616,6 +625,12 @@ public IEnumerable<T> Get<T>(IList<Document> documents, string collection) where | |||
return result; | |||
} | |||
|
|||
public Task<IEnumerable<T>> GetAsync<T>(long[] ids, string collection) where T : class |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as previous comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done! I've moved all obsolete method implementations from concrete classes to interface default implementations. The obsolete methods are now only defined once in the interfaces and can be easily removed in a future release. See commit dddbf52.
…ce default implementations Co-authored-by: MikeAlhayek <[email protected]>
…r binary compatibility Co-authored-by: sebastienros <[email protected]>
This PR addresses the binary breaking changes introduced by PR #627, which added
CancellationToken
parameters to several async methods but removed the original method signatures.Problem
PR #627 replaced existing async methods with new versions that include
CancellationToken
parameters:This caused binary compatibility issues where packages compiled against older versions of YesSql would fail at runtime because the original method signatures no longer existed.
Solution
This PR restores binary compatibility by adding back the original method signatures as obsolete overloads:
Changes Made
Interfaces
InitializeAsync()
andInitializeCollectionAsync(string)
SaveAsync
,GetAsync
,FlushAsync
,SaveChangesAsync
,CreateConnectionAsync
, andBeginTransactionAsync
FirstOrDefaultAsync
,ListAsync
,ToAsyncEnumerable
, andCountAsync
InitializeAsync
,InitializeCollectionAsync
, andGetNextIdAsync
Implementations
Additional Fixes
CancellationToken.None
Compatibility
The obsolete methods can be removed in a future major version release after giving users time to migrate to the CancellationToken-enabled versions.
Fixes #640.
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.