8000 Adding the ListUsers API by ChristopherLenz · Pull Request #58 · firebase/firebase-admin-dotnet · GitHub
[go: up one dir, main page]
Skip to content

Adding the ListUsers API #58

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 13 commits into from
Jun 14, 2019
Merged

Adding the ListUsers API #58

merged 13 commits into from
Jun 14, 2019

Conversation

ChristopherLenz
Copy link
Contributor

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

@hiranya911
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor

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_

@ChristopherLenz
cleanup of old solution fragments
89547e7
@ChristopherLenz
replaced the internal "JSON-representation-class" with a public one
4aa3127 
FirebaseAuth returns now:
 - DownloadAccountResponse --> ExportedUserRecords
 - UserRecord --> ExportedUserRecord
FirebaseAuth takes now ListUsersOptions
@ChristopherLenz
Copy link
Contributor Author

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
hiranya911 suggested changes May 24, 2019
View reviewed changes
Copy link
Contributor
@hiranya911 hiranya911 left a comment

Choose a reason for hiding this comment

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

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

@hiranya911 hiranya911 self-assigned this May 24, 2019
@ChristopherLenz
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
hiranya911 reviewed May 28, 2019
View reviewed changes
Copy link
Contributor
@hiranya911 hiranya911 left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

@hiranya911 is there any work to do for me?

@hiranya911
Copy link
Contributor

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
< 8000 div class="ml-n3 timeline-comment unminimized-comment comment previewable-edit js-task-list-container js-comment timeline-comment--caret" data-body-version="b64b0b3e5f147e516c969bd8a50e0ec1605e53196c0297dc2169096e82a3782b">
Copy link
Contributor Author
ChristopherLenz commented Jun 12, 2019 •
edited
Loading

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
hiranya911 suggested changes Jun 13, 2019
View reviewed changes
Copy link
Contributor
@hiranya911 hiranya911 left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

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

hiranya911
hiranya911 approved these changes Jun 14, 2019
View reviewed changes
Copy link
Contributor
@hiranya911 hiranya911 left a comment

Choose a reason for hiding this comment

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

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

FirebaseAdmin/FirebaseAdmin/Auth/UserRecord.cs
/// </summary>
public IUserInfo[] ProviderData { get; }

/// <summary>
Copy link
Contributor

Choose a reason for hiding this comment

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

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

@hiranya911 hiranya911 merged commit a1ee19c into firebase:master Jun 14, 2019
@hiranya911 hiranya911 mentioned this pull request Jun 17, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@hiranya911 hiranya911 hiranya911 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@hiranya911 hiranya911

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Request all users

2 participants

@ChristopherLenz @hiranya911
0