feat(Core): CNX-8352 operations.send #3094
Conversation
* ci: Show all warnings on CI builds * fix(all): Do not allow IDE0005 on CI * fix(sdk): IDE0005 Redundant Using statements in SDK projects (#3071) * fix(core): IDE0005 Redundant Using statements * fix(ci): GenerateDocumentationFile is required for IDE0005 to run on CI * fix(ci): Allow warnings on all CI jobs * fix(rhino/grasshopper): Fix IDE0005 Redundant Using Statements (#3072) * fix(gh): IDE0005 for Grasshopper connector * fix(rhino): IDE0005 for Rhino connector * fix(rh/gh): IDE0005 for Rhino/Grasshopper shared converter * tamed some warnings * Hushed more warnings * Eliminated many warnings in core * More fixes for Alan * Core fix * More changes * Base object serializer * fixed erroneous disposal * Reverted accidental change to blob constructor accessiblity * reverted switch needless statement change * Reverted some extra changes that weren't meant to be made * Supressed some extra resharper analysers * Deprecated serailizer v1 for sending * Deprecated send using v1 deserailize and offered cleaner overloads to send function * unnecessary using directive * fixed issue with checking struct for null --------- Co-authored-by: Alan Rynne <alan@speckle.systems>
| /// </example> | ||
| public static async Task<string> Send( | ||
| Base value, | ||
| ITransport transport, |
There was a problem hiding this comment.
The old send implementation had a way to disable sending to the SqlLite transport.
I know in our connectors we always want to default to that, but its not a good solution always.
In server side apps and automate functions, we specifically prefer not to use an sqllite cache. If it makes any sense to cache the objects, a memory transport is a much preferred way.
We should have a simple mechanism, like the useDefaultCache flag to have that flexibility.
There was a problem hiding this comment.
Gergo and I had a quick discussion about this. There's a couple things we can do to make it clearer which overloads can be used.
I'm going to make some changes and we can discuss the improvement
There was a problem hiding this comment.
@gjedlicska I've implemented the changes we discussed, + a couple of other minor tweaks following Alans suggestion.
The single transport overload now accepts a UseDefaultCache.
We've made it non-optional for a couple reasons.
1. We want caller to be explicit about the transports that are being used when sending.
2. It helps make the difference between the two overloads more explicit, and prevent misunderstandings
3. It also means our new overloads conflict less with existing ones, aiding backwards compat.
The overload that takes multiple transports has been renamed to Send to make the two overloads super clear.
| CancellationToken cancellationToken = default | ||
| ) | ||
| { | ||
| if (transport is null) |
There was a problem hiding this comment.
Is this necessary anymore? transport isn't nullable, calling this and ignoring the contract would be a bit daft. We're throwing anyways, without only slightly more context, meaning an NRE would be pretty obvious...
There was a problem hiding this comment.
I've changed this with my latest changes, we are no longer doing this check for transports.
However, I've still kept it in for value, since I'm not confident that it would throw lower down (we CAN serialize a null) but in the context of a send, we don't want users doing this.
If we were in .NET Standard2.1, we would want to mark the parameter as nullable, but use the [NotNull] to say that this function will always throw when a null is passed in. see https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/attributes/nullable-analysis
We can use Polysharp to bring this attribute back to us, but we haven't got that setup (tho, I believe we're no longer blocked on the CI side)
So for now, I think this is fine.
There was a problem hiding this comment.
I've changed it again.
The single transport version is truly nullable, with the useDefaultCache bool exposed. Now the null check is a feature, not a bug. And with it the DX should be improved.
| @@ -47,7 +46,7 @@ public static partial class Operations | |||
| /// </summary> | |||
| /// <param name="objects"></param> | |||
| /// <returns></returns> | |||
| [Obsolete("Please use the Serialize(Base @object) function. This function will be removed in later versions.")] | |||
| [Obsolete("Please use the Serialize(Base @object) function. This function will be removed in later versions.", true)] | |||
There was a problem hiding this comment.
Does this need to now be (Base value) ?
There was a problem hiding this comment.
It should be, I'll fix in another PR
| /// <exception cref="ArgumentException">The provided arguments are not valid</exception> | ||
| /// <exception cref="OperationCanceledException"></exception> | ||
| /// <exception cref="InvalidOperationException">The transport was in an invalid state</exception> | ||
| /// <exception cref="OperationCanceledException"><see cref="CancellationToken"/> requested cancel</exception> |
There was a problem hiding this comment.
Again, beyond scope but the commented out GetBlock() call below? Worth binning it while we are here?
There was a problem hiding this comment.
Depends whether you consider this a comment to stop developers adding a GetBlob function back later down the road.
I'll leave this decision up to @gjedlicska
|
I would have liked to make this change 100% non-breaking, allowing devs currently using Send to have their code still compile and work 100%. This will be true for most cases, there are a couple permutation of positional and keyword args where devs will now see a compiler error saying "parameter mismatch" or "Could not find suitable method, candidates are...".
Unless we are prepared to change the name of our new I would suggest that this shouldn't be a blocker. Any changes to overloads are technically breaking when you look at various positional keyword args. We have made far more breaking changes only last release with the inclusion of some extra functions in To get a flavour for how likely this is to affect users. All existing uses of |
AlanRynne
changed the title
As part of our effort to reduce analyser warnings across our code base. It's important that our Core functions are clear about what exceptions types they throw.
This motivation for this PR was to focus on
Operations.Sendand attempt to enforce and provide clear guidance on the exception types that can be thrown.Our original
Operations.Sendoverloads were doing some smelly things:In the signature of the function alone:
SerializerVerionenum to switch to using the old v1 serializer. This serializer hasn't been used (or tested) in almost two years, its past time we drop exposing it as an option forOperations.SendOnErrorActionto signal errors in transports and sending. We have since deprecated this in favour of throwing proper Exceptions instead. This property is effectively unused with our currentOperations.SenddisposeTransportsboolean (defaulting to false) that, when true, we would loop through and dispose anyIDisposabletransports that the user passes through. This doesn't follow any normal C# guidance on howIDisposablesshould be handled, and on top of that, we weren't disposing inside a finally block, so if an exception did occur, we weren't disposing the transports anyway. Thus this "feature" was not really usable, and much cleaner solved by expecting caller to handle the disposal using e.g. using ausingkeyword.And in the implementation:
In my new implementation, we have far fewer parameters, removing the obsolete and questionable designed ones.
We have also separated logging and error handing into one function
SendToTransportsand the actual send logic in an internalSendInternalfunction.