Adding the ListUsers API by ChristopherLenz · Pull Request #58 · firebase/firebase-admin-dotnet

ChristopherLenz · 2019-05-15T06:47:17Z

Added the "ListUsersAsync"-method to "FirebaseAuth"-class (resolves #51 )

hiranya911 · 2019-05-20T20:57:46Z

I believe this API should be based on Page and PagedEnumerable classes from Google GAX.

https://googleapis.github.io/google-cloud-dotnet/docs/guides/page-streaming.html

ChristopherLenz · 2019-05-21T12:49:32Z

I believe this API should be based on Page and PagedEnumerable classes from Google GAX.

https://googleapis.github.io/google-cloud-dotnet/docs/guides/page-streaming.html

I changed the paging-implementation. The drawback is that the previous internal classes used for the JSON-response have to be public. ATM I let them in the internal-folder. If this (internal --> public) is okay ,I would clean this up.

hiranya911 · 2019-05-21T17:59:49Z

Thanks @ChristopherLenz. The public API for this functionality should be something like this:

public PagedEnumerable<ExportedUserRecords, ExportedUserRecord> ListUsers(string pageToken)
public PagedEnumerable<ExportedUserRecords, ExportedUserRecord> ListUsers(string pageToken, CancellationToken cancellationToken)
public PagedEnumerable<ExportedUserRecords, ExportedUserRecord> ListUsers(String pageToken, int maxResults)
public PagedEnumerable<ExportedUserRecords, ExportedUserRecord> ListUsers(String pageToken, int maxResults, CancellationToken cancellationToken)

ExportedUserRecord and ExportedUserRecords should be new public types, and they should be the only types that get added to the public API surface.

It might also be easier to implement this by extending from the RestPagedEnumerable instead of writing our own from the scratch.

Then there's the question of asynchrony. All public APIs in the SDK are currently asynchronous. Ideally we should keep it that way. In that case it makes more sense to implement the APIs to return PagedAsyncEnumerable. But I'm not sure.

ChristopherLenz · 2019-05-22T13:27:35Z

Thanks @ChristopherLenz. The public API for this functionality should be something like this:

public PagedEnumerable<ExportedUserRecords, ExportedUserRecord> ListUsers(string pageToken)
public PagedEnumerable<ExportedUserRecords, ExportedUserRecord> ListUsers(string pageToken, CancellationToken cancellationToken)
public PagedEnumerable<ExportedUserRecords, ExportedUserRecord> ListUsers(String pageToken, int maxResults)
public PagedEnumerable<ExportedUserRecords, ExportedUserRecord> ListUsers(String pageToken, int maxResults, CancellationToken cancellationToken)

I added the pageToken, but removed the maxResults and cancellationToken. These parameters are used in subsequent method-calls on the pagedEnumerable itself.

var usersPage = firebaseAuth.ListUsersAsync(string.Empty);
var userPage = await usersPage.ReadPageAsync(10, source.Token)

ExportedUserRecord and ExportedUserRecords should be new public types, and they should be the only types that get added to the public API surface.

I will do this if you are satisfied with all other tasks.

It might also be easier to implement this by extending from the RestPagedEnumerable instead of writing our own from the scratch.

I did, but I'm not sure if the pageToken-handling is as expected. The requestProvider of RestPagedAsyncEnumerable is called for every request. This means that the pageToken is reset for every subsequent call on ReadPageAsync.

Then there's the question of asynchrony. All public APIs in the SDK are currently asynchronous. Ideally we should keep it that way. In that case it makes more sense to implement the APIs to return PagedAsyncEnumerable. But I'm not sure.

Yes I made everything async, because of the PagedEnumerable. I was not aware, that there is an async-version. Now it is async again.

hiranya911 · 2019-05-23T17:35:33Z

Thanks @ChristopherLenz. At a glance this looks correct. We just need the ExportedUserRecord types, some tests and clean up. I will take another thorough look, once those changes are in.

hiranya911 · 2019-05-23T21:24:47Z

Ok, having taken another look at other gcloud SDKs, I think our public API should be something like this:

public PagedAsyncEnumerable<ExportedUserRecords, ExportedUserRecord> ListUsersAsync(ListUsersOptions options)

class ListUsersOptions {
  public int? PageSize;
  public string PageToken;
}

For comparison with a similar API, see: https://googleapis.github.io/google-cloud-dotnet/docs/Google.Cloud.Storage.V1/api/Google.Cloud.Storage.V1.StorageClient.html#Google_Cloud_Storage_V1_StorageClient_ListBucketsAsync_System_String_Google_Cloud_Storage_V1_ListBucketsOptions_

FirebaseAuth returns now: - DownloadAccountResponse --> ExportedUserRecords - UserRecord --> ExportedUserRecord FirebaseAuth takes now ListUsersOptions

ChristopherLenz · 2019-05-24T08:06:24Z

Thanks for the tips @hiranya911. I've added all the requests you made. I don't know if I will find time to add some tests today.

hiranya911

Looks mostly good. I've left a few comments to work on. Plus we need tests to cover all new code.

extending userrecord added password hash and salt trailing newlines in files class documentation renamed page-manager and service-request to match the API-method

hiranya911

This is starting to look good. There are few more changes that I'd like to make based on the API feedback I've received from the team, but I can do those changes myself, once this PR is merged. But we need some tests before we can proceed any further with this.

ChristopherLenz · 2019-06-11T08:12:34Z

@hiranya911 is there any work to do for me?

hiranya911 · 2019-06-11T18:58:34Z

Hi @ChristopherLenz. This is still lacking tests for some of the functionality. Ideally we need to test that iterating over users via PagedAsyncEnumerator works as expected.

ChristopherLenz · 2019-06-12T07:28:57Z

Hi @ChristopherLenz. This is still lacking tests for some of the functionality. Ideally we need to test that iterating over users via PagedAsyncEnumerator works as expected.

Okay I've added a test for that. Please let me know if it is okay now.

hiranya911

Thanks @ChristopherLenz. This looks good. I just had 2 points to make. Also please sync with the latest master branch. There are many changes in UserRecord and a few other classes that are relevant to this PR.

ChristopherLenz · 2019-06-14T07:13:45Z

@hiranya911
So I think that's all now :-)

hiranya911

This needs a bit more clean up. But I can find some time to work on it after we merge this. Thanks,

hiranya911 · 2019-06-14T22:41:49Z

        /// </summary>
        public IUserInfo[] ProviderData { get; }

+        /// <summary>


Sorry if I wasn't clear in my earlier review. But these properties are not needed. CreatedAt and LastLoginAt are already in UserMetadata and ValidSince is same as TokensValidAfterTimestamp

ChristopherLenz added 2 commits May 10, 2019 13:16

added a method to list firebase-users page wise

fb974a3

added a test for "ListUsers"-method

a7c0407

replaced the custom page-implementation with Google's PagedEnumerable

ff90d61

replaced the PagedEnumerable by RestPagedAsyncEnumerable

c932fe7

ChristopherLenz added 2 commits May 24, 2019 08:18

cleanup of old solution fragments

89547e7

replaced the internal "JSON-representation-class" with a public one

4aa3127

FirebaseAuth returns now: - DownloadAccountResponse --> ExportedUserRecords - UserRecord --> ExportedUserRecord FirebaseAuth takes now ListUsersOptions

hiranya911 suggested changes May 24, 2019

View reviewed changes

hiranya911 self-assigned this May 24, 2019

resolved all remarks

c12f0eb

extending userrecord added password hash and salt trailing newlines in files class documentation renamed page-manager and service-request to match the API-method

hiranya911 reviewed May 28, 2019

View reviewed changes

Comment thread FirebaseAdmin/FirebaseAdmin/Auth/ExportedUserRecord.cs Outdated

Comment thread FirebaseAdmin/FirebaseAdmin/Auth/FirebaseUserManager.cs Outdated

ChristopherLenz added 2 commits May 29, 2019 07:44

moved createdAt, lastLoginAt & validSince into UserRecord

b5cd7c7

added tests for ListUsersRequest

0dedee7

further test for ListUsers with RestPagedAsync

a55bfef

hiranya911 suggested changes Jun 13, 2019

View reviewed changes

Comment thread FirebaseAdmin/FirebaseAdmin.Tests/Auth/FirebaseUserManagerTest.cs Outdated

Comment thread FirebaseAdmin/FirebaseAdmin.Tests/Auth/FirebaseUserManagerTest.cs Outdated

Comment thread FirebaseAdmin/FirebaseAdmin/Auth/UserRecord.cs Outdated

ChristopherLenz added 2 commits June 14, 2019 08:54

moved ListUsers-logic to FirebaseUserManager

c16710d

Merge remote-tracking branch 'upstream/master' into list-users

24991d3

tests got lost during merge

8ff4b8f

hiranya911 approved these changes Jun 14, 2019

View reviewed changes

hiranya911 merged commit a1ee19c into firebase:master Jun 14, 2019

hiranya911 mentioned this pull request Jun 17, 2019

Updating the ListUsers() Implementation #77

Merged

Conversation

ChristopherLenz commented May 15, 2019

Uh oh!

hiranya911 commented May 20, 2019

Uh oh!

ChristopherLenz commented May 21, 2019

Uh oh!

hiranya911 commented May 21, 2019

Uh oh!

ChristopherLenz commented May 22, 2019

Uh oh!

hiranya911 commented May 23, 2019

Uh oh!

hiranya911 commented May 23, 2019

Uh oh!

ChristopherLenz commented May 24, 2019

Uh oh!

hiranya911 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hiranya911 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ChristopherLenz commented Jun 11, 2019

Uh oh!

hiranya911 commented Jun 11, 2019

Uh oh!

ChristopherLenz commented Jun 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hiranya911 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ChristopherLenz commented Jun 14, 2019

Uh oh!

hiranya911 left a comment

Choose a reason for hiding this comment

Uh oh!

hiranya911 Jun 14, 2019

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ChristopherLenz commented Jun 12, 2019 •

edited

Loading