Add support for bulk user exports #152

mfb2 · 2018-09-19T15:36:48Z

This commit adds the ability to create bulk user exports. With this
update, a new request type of StatusCodeRequest was added in order to
handle queries against the job_error endpoint which report the error
result of a given job.

Please note: With this change, consumers of the API will be responsible
for fetching and decompressing the gzip file produced from the job
request via the URL provided in the response.

mfb2 · 2018-09-19T15:38:21Z

This PR addresses issue #95 . Please let me know if you have questions, comments, concerns, or suggestions on this PR, I'm happy to assist.

Thank you!

mfb2 · 2018-09-19T22:00:40Z

src/main/java/com/auth0/net/StatusCodeRequest.java

+            final String payload = body.string();
+            return new JobError(response.code(), payload);
+        } catch (final IOException e) {
+            throw new APIException("Failed to parse response", response.code(), e);


I'm having a tough time mocking an appropriate response object that will force a throw on this line. This is causing CI coverage checks to fail due to insufficient coverage. Response and MockResponse are final classes, so they can't be sub-classed or modified to produce an IOException in order to fall into this block.

Thoughts?

Any suggestions on ways to proceed?

Skipping since class will disappear

mfb2 · 2018-09-19T22:02:21Z

src/main/java/com/auth0/json/mgmt/jobs/UsersExport.java

+    @JsonProperty("limit")
+    private int limit;
+    @JsonProperty("fields")
+    private List<List<Map<String, String>>> fields = null;


I realize this structure is a bit yucky... This is due to the spec provided by Auth0's API docs; the sample JSON response has an array within an array, which in turn contains a collection of objects that represent the field information.

Better to move this into a new class. Lets create the UserExportField class with two string properties:

name (required, should be part of the constructor)

export_as (optional, available via setter).

Then this class (let's rename it to UserExportOptions) will have as fields: List<UserExportField> fields. I'm confident the double array schema you see in the API docs is wrong. But will need to try this manually once you make the changes to be sure.

mfb2 · 2018-09-19T22:04:38Z

src/main/java/com/auth0/net/StatusCodeRequest.java

+
+import java.io.IOException;
+
+public class StatusCodeRequest extends EmptyBodyRequest<JobError> {


I created a new request type and corresponding JobError object in order to provide the ability to inspect the HTTP status code returned by the job_error API endpoint. There didn't appear to be any other way to get that info aside from manually mapping the HTTP status code into the field directly as part of the response parsing.

The http status code is irrelevant. It will be a 200 plus the error description (if job failed) in the body of the response.
This class should be deleted and instead the caller method should return a Request<JobError>

This commit adds the ability to create bulk user exports. With this update, a new request type of `StatusCodeRequest` was added in order to handle queries against the `job_error` endpoint which report the error result of a given job. Please note: With this change, consumers of the API will be responsible for fetching and decompressing the `gzip` file produced from the job request via the URL provided in the response.

lbalmaceda

Thanks for the contribution and sorry for the delay. I've left you my initial assessment of the export functionality. I had to check with the backend team to ensure the API docs were correct. Anyway, I'll wait to hear back from you before reviewing in detail the tests.

lbalmaceda · 2019-01-29T18:49:44Z

src/main/java/com/auth0/client/mgmt/JobsEntity.java

+     *
+     * @return A request to execute the job.
+     */
+    public Request<Job> exportUsers() { return this.exportUsers(new UsersExport()); }


This signature is not needed. I can't imagine a scenario where I'd call the endpoint without any parameters

lbalmaceda · 2019-01-29T18:51:57Z

src/main/java/com/auth0/client/mgmt/JobsEntity.java

+        final int limit = usersExport.getLimit();
+        final List<List<Map<String, String>>> fields = usersExport.getFields();
+
+        if (this.isValued(connectionId)) {


All the null-checks below this line should belong to the UserExport class. Any required properties should be set on instantiation and the constructor should validate them. Here you only need to check that the UserExport instance is not null like you're doing in the first line. Later when constructing the request, null values would be ignored by the function that inserts the values into the request.

lbalmaceda · 2019-01-29T19:52:25Z

src/main/java/com/auth0/client/mgmt/JobsEntity.java

+        return request;
+    }
+
+    private StatusCodeRequest generateJobErrorRequest(final String url) {


This is called only once. I don't see the point on extracting it into a new method.

lbalmaceda · 2019-01-29T19:53:02Z

src/main/java/com/auth0/client/mgmt/JobsEntity.java

        request.addHeader("Authorization", "Bearer " + apiToken);
        request.setBody(requestBody);
        return request;
    }
+
+    private CustomRequest<Job> generateJobRequest(final String url) {


This is called only once. I don't see the point on extracting it into a new method.

lbalmaceda · 2019-01-29T19:53:36Z

src/main/java/com/auth0/client/mgmt/JobsEntity.java

+                .toString();
+    }
+
+    private boolean isValued(final String string) {


Remove this method and use Asserts.assertNotNull(propertyValue, "property description") instead. Any invalid value further than a null can be considered user error and will be ultimately validated by the server.

lbalmaceda · 2019-02-01T20:38:54Z

src/main/java/com/auth0/net/StatusCodeRequest.java

+            final String payload = body.string();
+            return new JobError(response.code(), payload);
+        } catch (final IOException e) {
+            throw new APIException("Failed to parse response", response.code(), e);


Skipping since class will disappear

lbalmaceda · 2019-02-01T20:48:42Z

src/test/java/com/auth0/client/mgmt/JobsEntityTest.java

+        final Map<String, Object> body = bodyFromRequest(recordedRequest);
+        assertThat(body.size(), is(4));
+        assertThat(body, hasEntry("connection_id", CONNECTION_ID));
+        assertThat(body, hasEntry("format", CSV));


you should check that the fields and limit properties are also present with the correct values (Applies to all similar tests)

lbalmaceda · 2019-02-01T20:52:09Z

src/main/java/com/auth0/client/mgmt/JobsEntity.java

+     * @param usersExport Object containing the parameters by which the user export job will be run.
+     * @return A request to execute the job.
+     */
+    public Request<Job> exportUsers(final UsersExport usersExport) {


Let's name it requestUsersExport

lbalmaceda · 2019-02-01T21:13:19Z

src/main/java/com/auth0/client/mgmt/JobsEntity.java

+     * @param jobId Id of the job to be retrieved
+     * @return A request to execute the job details retrieval.
+     */
+    public Request<Job> getJob(final String jobId) {


Normally I'd say get is enough but since this class is handling many types of errors and requests, I think you made a good call here.

lbalmaceda · 2019-02-01T21:22:30Z

src/main/java/com/auth0/json/mgmt/jobs/UsersExport.java

+    @JsonProperty("limit")
+    private int limit;
+    @JsonProperty("fields")
+    private List<List<Map<String, String>>> fields = null;


Better to move this into a new class. Lets create the UserExportField class with two string properties:

name (required, should be part of the constructor)

export_as (optional, available via setter).

Then this class (let's rename it to UserExportOptions) will have as fields: List<UserExportField> fields. I'm confident the double array schema you see in the API docs is wrong. But will need to try this manually once you make the changes to be sure.

lbalmaceda · 2019-02-12T14:32:27Z

Hey @mfb2, are you still interested in this feature? Don't hesitate asking any follow up questions.

mfb2 · 2019-02-12T16:52:50Z

I am still interested in the feature. I had been waiting for this PR to be reviewed since September last year, and my project has since moved on. We had to fork this project at an earlier version in order to finish our work. I do not have time to address the volume of feedback as I am no longer funded to do development on this product.

I believe in open source and a vibrant community of contributions; our challenge here appears to be in aligning expectations. When resources permit, I will push an update to incorporate your feedback. In the meantime, I encourage you to leverage my contribution as a quick-start for completing the feature.

cocojoe · 2019-02-25T10:21:32Z

@mfb2 We appreciate your contribution, apologise for the time taken to action it originally. I agree it may make a good quick-start as you put it. However, we have a lot of repos and don't typically pick up and continue another authors work unless it aligns with our own priorities. I'm happy to leave this PR open for a while as it would be great to see you complete your contribution 👍

cocojoe · 2019-10-10T12:55:09Z

Closed due to inactivity.

mfb2 force-pushed the support_users_exports branch 3 times, most recently from f01153d to 096c30e Compare September 19, 2018 17:19

mfb2 commented Sep 19, 2018

View reviewed changes

mfb2 force-pushed the support_users_exports branch from 096c30e to d859dfc Compare October 24, 2018 16:14

lbalmaceda suggested changes Feb 1, 2019

View reviewed changes

cocojoe closed this Oct 10, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for bulk user exports #152

Add support for bulk user exports #152

mfb2 commented Sep 19, 2018

mfb2 commented Sep 19, 2018

mfb2 Sep 19, 2018

lbalmaceda Feb 1, 2019

mfb2 Sep 19, 2018

lbalmaceda Feb 1, 2019

mfb2 Sep 19, 2018

lbalmaceda Feb 1, 2019

lbalmaceda left a comment

lbalmaceda Jan 29, 2019

lbalmaceda Jan 29, 2019

lbalmaceda Jan 29, 2019

lbalmaceda Jan 29, 2019

lbalmaceda Jan 29, 2019

lbalmaceda Feb 1, 2019

lbalmaceda Feb 1, 2019

lbalmaceda Feb 1, 2019

lbalmaceda Feb 1, 2019

lbalmaceda Feb 1, 2019

lbalmaceda commented Feb 12, 2019

mfb2 commented Feb 12, 2019

cocojoe commented Feb 25, 2019

cocojoe commented Oct 10, 2019


		import java.io.IOException;

		public class StatusCodeRequest extends EmptyBodyRequest<JobError> {

Add support for bulk user exports #152

Add support for bulk user exports #152

Conversation

mfb2 commented Sep 19, 2018

mfb2 commented Sep 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lbalmaceda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lbalmaceda commented Feb 12, 2019

mfb2 commented Feb 12, 2019

cocojoe commented Feb 25, 2019

cocojoe commented Oct 10, 2019