Added support for CheckpointPagingInformation on Connections.GetAllAsync #678
Conversation
LaurentLardenois
had a problem deploying
to
external
GitHub Actions
Failure
Changed private to public accessor which I accidentally set wrongfully.
Changed some mistakes
frederikprijck
added
the
waiting for customer
…ePaginationInfo class. Current implementation only exists for the ConnectionsClient.GetAllAsync call
| queryStrings["from"] = checkpointPagination.From; | ||
| queryStrings["take"] = checkpointPagination.Take.ToString(); | ||
| break; | ||
| } |
There was a problem hiding this comment.
We shouldnt need this. All we had to avoid is introducing the breaking change. We have no need for BasePaginationInfo or such a switch case.
Let's just add a new method for the checkpoint pagination where checkpoint pagination is required, like we do here (but the other way around): https://github.com/auth0/auth0.net/blob/master/src/Auth0.ManagementApi/Clients/ClientGrantsClient.cs#L107-L153
There was a problem hiding this comment.
What is the software design choice to use an overload, but also use one with defaults? There seems to be a convolution of different architecture choices. I would argue that with the base class it stays more in line with a singular way of working with defaults, which seems to be the current 'standard'. For instance that link you provide were the situation is 'the other way around' already is a different way of working since the other overload is the default. I know that switching everything to the baseclass variant with these changes also changes a bit, but in my opinion the base class stays more in line with the way of working with defaults. Let me know your thoughts!
There was a problem hiding this comment.
Also, calling Connection.GetAllAsync(request, null, cancellationToken) will result in an ambiguous overload error. Which is also the case with the ClientGrantsClient.GetAllOrganizationsAsync call.
There was a problem hiding this comment.
See here the example, so that would mean that with the change you suggested, it would still be a breaking change for everybody that explicitly sets the value of pagination to null.
There was a problem hiding this comment.
Why could we not stick to what you had initially, but avoid throwing the error if the paging was null?
There was a problem hiding this comment.
Because that would still result in an ambiguous invocation (unless you want to upgrade the whole SDK to a framework version that supports nullable types and make only one overload nullable that way, and I'm not even sure that would work). The issue is that the sdk has not been designed to work with overloads properly, and therefore we should work towards a way that makes these ambiguous calls impossible and forces a strict way of working. For now the best way to have it not breaking and allow for future expansion is in my opinion to work with the base class with default null and type switched branching for implementation. I'm not really a big fan of default values for this exact reason. These issues would never have surfaced if from the beginning there were stricter overloads that never accepted null, because than this would never break the backwards compatibility. But for now I think this latest version with the base class (I hate empty abstract base classes and interfaces so believe me I chose the lesser of the evils here) is the best solution we have.
There was a problem hiding this comment.
Having looked into this, I believe there is no way to implement this in a non breaking way unless we rework how we do the overloads. I believe reworking the overloads is not the scope of this PR, and something we want to dedicate a separate PR for.
The only thing I can see us doing with this PR today is revert the last two commits (we need to revert f1e0344 as well, because that is general guidance and not related to this change) and ensure things compile (it does not compile when you revert those last two commits because you use CheckpointPagingInformation instead of CheckpointPaginationInfo).
Making that change will result in a compilation error for anyone that is passing null explicitly and that could be acceptable. We try and stick to semver wherever possible, but compile time breaking changes are arguably acceptable in certain situations. However, as demand is low on this specific request, I would prefer to avoid doing that.
The issue is that the sdk has not been designed to work with overloads properly
I believe our overloads are not bad, but it's not possible to introduce this in a non breaking way without an internal refactor so there is definitely an opportunity to improve. Just not in this PR. If you feel strong about this, I am happy to review a PR that reworks this in the entire codebase (there shouldnt be that many occurences though, but lets ensure we discuss implementation details before doing the work) and we can hold of merging this PR until we merged that refactor. But it is not something I can prioritize myself at the moment.
That would also mean we can not introduce support for CheckpointPagination in the Connections.GetAllAsync method without doing the refactor first, unless you can think of a way that would allow for us to introduce this in a non breaking way without introducing something new only for this use-case.
There was a problem hiding this comment.
And to be clear, I appreciate the work you put in this and the idea you bring to the table about the pagination. I just think we should not just randomly change how we handle pagination overloads in a PR like this, but properly consider how we want to implement this going forward in a non breaking way for all methods having support for both pagination approaches.
But as mentioned, this is not something I can prioritize myself for the time being.
There was a problem hiding this comment.
Since reverting this current PR to the suggested commit would introduce breaking changes for people, I think changing this PR into the pagination refactor everywhere is a good idea. The question than is, what is the preference? With the base class it would not lead to a breaking change anywhere, even were people are currently using the overloads, however if we turn everything into overloads it will introduce breaking changes, and while I would argue this is the better solution from a software architecture standpoint, deviating only with this would make the code base more inconsistent in my opinion.
So I leave it up to you to choose between refactoring all pagination functionality with either non nullable overloads or a base class for the pagination. Note that in case of the base class I will have to look on an endpoint to endpoint basis if checkpoint pagination is allowed and make sure to put it in the documentation of the function and throw not supported exceptions if checkpoint pagination info is passed to the function, or you can choose to leave the strict pagination type intact in places that do not support checkpoint pagination yet.
Let me know your thoughts!
|
Given the discussion we had, I am going to close this PR. Feel free to open an issue to discuss the pagination changes if you want. Regardless, I appreciate the effort you put in this, thanks! |
LaurentLardenois
requested a deployment
to
external
GitHub Actions
Waiting
Added support for CheckpointPagingInformation on Connections.GetAllAsync to support the management api feature according to the docs (https://auth0.com/docs/api/management/v2/connections/get-connections). Added a base function with overloads and extra documentation for clarification. These changes allow the retrieval of all connections on tenants with >1000 connections. To test just try to receive connections on a tenant with all three overloads.