Make the session adapter class public #6526
Conversation
|
I still prefer the current version I must say because it ressembles very closely to the v8 version except the usage of the GetAdaptedSession extension method. Everything else is exactly the same as the v8 version and I like that But I agree the adapter implementing also the Completable interface could be a bit confusing In case other's prefer this variant I won't hold things up though |
danielmarbach
force-pushed
the
public-adapter
branch
from
febdda0
to
8539c94
Compare
|
under the assumption that this has no/little impact on the downstream usage (TxSession), I think I prefer this approach as it the safer approach and avoids any potential confusion due to the casting that happens in code and the persistence-specific extension methods. I think the downside of this approach is slightly different from v8 is acceptable, the API usage matches the v8 version, it's just a different type being used. This PR is currently missing the obsolete markers on he adapter class though. |
|
How about but then if we do that we technically also need to add the obsoleted class to the v8 obsoletes... I wonder if this is really necessary for such an internal change |
I think it would be ok to just remove the class with v8? |
|
Agreed |
| /// </summary> | ||
| /// <param name="session">The storage session.</param> | ||
| public static CompletableSynchronizedStorageSession GetAdaptedSession(this CompletableSynchronizedStorageSession session) => | ||
| ((CompletableSynchronizedStorageSessionAdapter)session).AdaptedSession; |
There was a problem hiding this comment.
@timbussmann We discussed this during this morning's sync. I agree this looks very confusing, but keeping v7 and v8 aligned should be a priority, especially due to how we work and how much context is lost around these changes.
Would it be helpful if we modify this as proposed in #6529 ?
|
Discussed this again and indeed this approach is superior because it avoids the scenario of being able to inject. I did double check the ReceiveComponent registration and we are no longer providing a binding of |
|
I'm assuming we are still ok with just removing the class in v8 since it is such an internal concept |
* Introduce SynchronizedStorage feature Bring in feature dependency Approve API * Adapter to bridge the old and the new world * Make it possible to retrieve the storage session via DI and also open the completable one from outside the core * Add acceptance test that verifies session can be opened outside the pipeline * Additional checks * Shorter endpoint name * Reregister the CompletableSynchronizedStorageSession properly so that we don't override Co-authored-by: Tomasz Masternak <tomasz.masternak@particular.net> * Fix synchronized storage retrival * Update src/NServiceBus.Core/Pipeline/Incoming/LoadHandlersConnector.cs Co-authored-by: Tim Bussmann <timbussmann@users.noreply.github.com> * make GetAdaptedSession extension public * Session disposal (#6525) * resolve storage session per call * only dispose session once * approve scope change in test * change registration back to same scope as the referenced type * Use GetAdaptedSession consistently * Make the session adapter class public (#6526) * make the session adapter class public * Approve Co-authored-by: Daniel Marbach <daniel.marbach@openplace.net> * Move CompletableSynchronizedStorageSessionAdapter into the persistence namespace * Rename paramater * aligning names Co-authored-by: Tomasz Masternak <tomasz.masternak@particular.net> Co-authored-by: Tim Bussmann <timbussmann@users.noreply.github.com>
As discussed on yesterday's sync, this is a quick attempt of making the adapter class public instead of directly implementing and wrapping
CompleteableSynchronizedStorageSessionto reduce potential confusion about the specific implementation of the interface.