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.

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

Added support for CheckpointPagingInformation on Connections.GetAllAsync #678

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 14 additions & 5 deletions src/Auth0.ManagementApi/Clients/ConnectionsClient.cs
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ public Task<Connection> GetAsync(string id, string fields = null, bool includeFi
/// <param name="pagination">Specifies pagination info to use when requesting paged results.</param>
/// <param name="cancellationToken">The cancellation token to cancel operation.</param>
/// <returns>An <see cref="IPagedList{Connection}"/> containing the list of connections.</returns>
public Task<IPagedList<Connection>> GetAllAsync(GetConnectionsRequest request, PaginationInfo pagination = null, CancellationToken cancellationToken = default)
public Task<IPagedList<Connection>> GetAllAsync(GetConnectionsRequest request, BasePaginationInfo pagination = null, CancellationToken cancellationToken = default)
{
if (request == null)
throw new ArgumentNullException(nameof(request));
Expand All @@ -104,9 +104,18 @@ public Task<IPagedList<Connection>> GetAllAsync(GetConnectionsRequest request, P

if (pagination != null)
{
queryStrings["page"] = pagination.PageNo.ToString();
queryStrings["per_page"] = pagination.PerPage.ToString();
queryStrings["include_totals"] = pagination.IncludeTotals.ToString().ToLower();
switch (pagination)
{
case PaginationInfo offsetPagination:
queryStrings["page"] = offsetPagination.PageNo.ToString();
queryStrings["per_page"] = offsetPagination.PerPage.ToString();
queryStrings["include_totals"] = offsetPagination.IncludeTotals.ToString().ToLower();
break;
case CheckpointPaginationInfo checkpointPagination:
queryStrings["from"] = checkpointPagination.From;
queryStrings["take"] = checkpointPagination.Take.ToString();
break;
}
Copy link
Member

@frederikprijck frederikprijck Nov 28, 2023

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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!

Copy link
Author

Choose a reason for hiding this comment

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

Also, calling Connection.GetAllAsync(request, null, cancellationToken) will result in an ambiguous overload error. Which is also the case with the ClientGrantsClient.GetAllOrganizationsAsync call.

Copy link
Author

Choose a reason for hiding this comment

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

image
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.

Copy link
Member

Choose a reason for hiding this comment

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

Why could we not stick to what you had initially, but avoid throwing the error if the paging was null?

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

@frederikprijck frederikprijck Nov 28, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Member

@frederikprijck frederikprijck Nov 28, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Author

@LaurentLardenois LaurentLardenois Nov 29, 2023

Choose a reason for hiding this comment

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

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!

}

// Add each strategy as a separate querystring
Expand Down Expand Up @@ -144,4 +153,4 @@ public Task CheckStatusAsync(string id, CancellationToken cancellationToken = de
return Connection.GetAsync<object>(BuildUri($"connections/{EncodePath(id)}/status"), DefaultHeaders, cancellationToken: cancellationToken);
}
}
}
}
2 changes: 1 addition & 1 deletion src/Auth0.ManagementApi/Clients/IConnectionsClient.cs
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ public interface IConnectionsClient
/// <param name="pagination">Specifies pagination info to use when requesting paged results.</param>
/// <param name="cancellationToken">The cancellation token to cancel operation.</param>
/// <returns>An <see cref="IPagedList{Connection}"/> containing the list of connections.</returns>
Task<IPagedList<Connection>> GetAllAsync(GetConnectionsRequest request, PaginationInfo pagination = null, CancellationToken cancellationToken = default);
Task<IPagedList<Connection>> GetAllAsync(GetConnectionsRequest request, BasePaginationInfo pagination = null, CancellationToken cancellationToken = default);

/// <summary>
/// Updates a connection.
Expand Down
9 changes: 9 additions & 0 deletions src/Auth0.ManagementApi/Paging/BasePaginationInfo.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
namespace Auth0.ManagementApi.Paging
{
/// <summary>
/// This base class is needed to support both offset and checkpoint pagination with default constructors
/// </summary>
public abstract class BasePaginationInfo
{
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ namespace Auth0.ManagementApi.Paging
/// <summary>
/// Specifies checkpoint pagination info to use when requesting paged results.
/// </summary>
public class CheckpointPaginationInfo
public class CheckpointPaginationInfo : BasePaginationInfo
{
/// <summary>
/// Initializes a new instance of the <see cref="CheckpointPaginationInfo"/> class.
Expand Down
4 changes: 2 additions & 2 deletions src/Auth0.ManagementApi/Paging/PaginationInfo.cs
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,10 @@
/// <summary>
/// Specifies pagination info to use when requesting paged results.
/// </summary>
public class PaginationInfo
public class PaginationInfo : BasePaginationInfo
{
/// <summary>
/// Initializes a new instance of the <see cref="PaginationInfo"/> class.
/// Initializes a new instance of the <see cref="PaginationInfo"/> class for offset pagination.
/// </summary>
/// <param name="pageNo">Page index of the results to return. First page is 0.</param>
/// <param name="perPage">Number of results per page.</param>
Expand Down