Resource server #77

mfarsikov · 2017-09-12T07:25:39Z

Added Resource server CRUD

For ease building requests RequestBuilder was added, and refactored already existed Entities to use it.

lbalmaceda

Hey. Thanks for your contribution! Overall it looks good. Apart from the comments I left you, I'll need 2 big things:

Please keep the main purpose of the PR and remove the refactor of the Request creation and handling. You can move those commits to a separate PR if you want, but they add noise here.
Please add the missing tests as some endpoints are not tested.

Once assorted, I'll carefully check each endpoint against the server to see if we're missing any required parameters.
Cheers

lbalmaceda · 2017-11-09T17:31:25Z

.gitignore

@@ -101,6 +101,7 @@ gradle-app.setting
 .idea/sqlDataSources.xml
 .idea/dynamic.xml
 .idea/uiDesigner.xml
+/.idea/


We decided to keep some of the idea files, by using the "intellij+iml" configuration provided by gitignore.io. Remove this line please.

lbalmaceda · 2017-11-09T17:32:29Z

src/main/java/com/auth0/json/mgmt/ResourceServer.java

+    }
+
+    @Override
+    public String toString() {


Remove this method as we chose not to provide it on the other entities.

lbalmaceda · 2017-11-09T17:32:37Z

src/main/java/com/auth0/json/mgmt/Scope.java

+    }
+
+    @Override
+    public String toString() {


Remove this method as we chose not to provide it on the other entities.

lbalmaceda · 2017-11-09T17:33:20Z

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

+import static org.hamcrest.Matchers.notNullValue;
+import static org.junit.Assert.assertThat;
+
+public class ResourceServerEntityTest extends BaseMgmtEntityTest {


More tests should be added to cover all the new methods.

lbalmaceda · 2017-11-09T17:41:43Z

src/main/java/com/auth0/json/mgmt/ResourceServer.java

+    @JsonProperty("is_system")
+    private Boolean isSystem;
+
+    public ResourceServer() {


I don't remember if this constructor must exist because of Jackson, see if it can be removed. Also we need a constructor that receives at least the identifier parameter as that's the only required parameter when creating a new ResourceServer instance to post. id will always be returned by the server, but I trust Jackson is going to handle it just fine 😄 . (Add those tests)

Default constructor removed.
As I understand JSON transformation is covered in ResourceServerEntityTest when content of resource_servers_list.json is converted to ResourceServer instances. Do we nee additional tests for JSON conversion?

lbalmaceda · 2017-11-09T17:44:35Z

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

+     * Cretes request for fetching single resource server by it's ID.
+     * See <a href=https://auth0.com/docs/api/management/v2#!/Resource_Servers/get_resource_servers_by_id>API documentation</a>
+     *
+     * @param id {@link ResourceServer#identifier} field


id and identifier are different properties of a ResourceServer. My guess is that the id is the one the server assigns when it's created, and the identifier is the name we choose for it (like "myapp.auth0.com"). An identifier is required when creating the ResourceServer while the id is always returned when listing/getting resource servers.

thanks, fixed

lbalmaceda · 2017-11-09T17:45:19Z

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

+     * @param id {@link ResourceServer#identifier} field
+     * @return request to execute
+     */
+    public Request<ResourceServer> get(String id) {


Rename to resourceServerId

lbalmaceda · 2017-11-09T17:45:34Z

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

+     * @param id {@link ResourceServer#identifier} field
+     * @return request to execute
+     */
+    public Request<Void> delete(String id) {


Rename to resourceServerId

lbalmaceda · 2017-11-27T14:24:43Z

RequestBuilder and RequestUnderConstruction classes are still there.. Please remove them and just add the Resource Server entity and its tests.

mfarsikov · 2017-11-27T21:00:57Z

RequestBuilder removed :(

lbalmaceda

@mfarsikov Thanks. Like I said, feel free to submit those improvements in a separate PR and I'll review them in the future. I've some new feedback for you:

Test setters as coverage complains you're not testing them.
I've doubts with verificationKey and verificationLocation and can't make the server return them, maybe they are just set on creation. I'll ask the team to verify they are correct. Edit: Added a comment to remove the verificationKey property.

lbalmaceda · 2017-11-28T14:41:57Z

src/main/java/com/auth0/json/mgmt/Scope.java

+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonProperty;
+
+public class Scope {


Add these annotations at the class level

@SuppressWarnings("WeakerAccess", "unused") @JsonIgnoreProperties(ignoreUnknown = true) @JsonInclude(JsonInclude.Include.NON_NULL)

lbalmaceda · 2017-11-28T15:19:44Z

src/main/java/com/auth0/json/mgmt/ResourceServer.java

+    private Boolean allowOfflineAccess;
+    @JsonProperty("skip_consent_for_verifiable_first_party_clients")
+    private Boolean skipConsentForVerifiableFirstPartyClients;
+    @JsonProperty("token_lifetime")


It's missing the "token_lifetime_for_web" property.

lbalmaceda · 2017-11-28T15:20:21Z

src/main/java/com/auth0/json/mgmt/ResourceServer.java

+@JsonIgnoreProperties(ignoreUnknown = true)
+@JsonInclude(JsonInclude.Include.NON_NULL)
+public class ResourceServer {
+    @JsonProperty


add the explicit property name in the annotation

lbalmaceda · 2017-11-28T15:20:25Z

src/main/java/com/auth0/json/mgmt/ResourceServer.java

+public class ResourceServer {
+    @JsonProperty
+    private String id;
+    @JsonProperty


add the explicit property name in the annotation

lbalmaceda · 2017-11-28T15:21:47Z

src/main/java/com/auth0/json/mgmt/ResourceServer.java

+    @JsonProperty("scopes")
+    private List<Scope> scopes;
+    @JsonProperty("signing_alg")
+    private String signingAlg;


rename to signingAlgorithm (same for getters/setters)

lbalmaceda · 2017-11-28T15:53:22Z

src/main/java/com/auth0/json/mgmt/ResourceServer.java

+    }
+
+    @JsonCreator
+    public ResourceServer(@JsonProperty("id") String id,


Remove this constructor as the only required field is "identifier". Add another one with no-args, since when you need to PATCH the resource server you can't specify the "identifier".

lbalmaceda · 2017-11-28T15:53:51Z

src/main/java/com/auth0/json/mgmt/ResourceServer.java

+    @JsonProperty("is_system")
+    private Boolean isSystem;
+
+    public ResourceServer(String identifier) {


Annotate this constructor with @JsonCreator and add the @JsonProperty annotation to the "identifier" parameter.

lbalmaceda · 2017-11-28T15:56:36Z

src/main/java/com/auth0/json/mgmt/ResourceServer.java

+        return identifier;
+    }
+
+    public void setIdentifier(String identifier) {


Remove this setter as it cannot change.

lbalmaceda · 2017-11-28T16:13:23Z

src/main/java/com/auth0/json/mgmt/ResourceServer.java

+        this.name = name;
+    }
+
+    public Boolean getIsSystem() {


Rename to isSystem

lbalmaceda · 2017-11-28T16:21:47Z

src/main/java/com/auth0/json/mgmt/Scope.java

+    private String value;
+
+    @JsonCreator
+    public Scope(@JsonProperty("value") String value,


Value is the only required property. Let's have a single constructor with value instead of both.

lbalmaceda · 2017-11-28T17:36:32Z

src/main/java/com/auth0/json/mgmt/ResourceServer.java

+    @JsonProperty("token_lifetime")
+    private Integer tokenLifetime;
+    @JsonProperty("verificationKey")
+    private String verificationKey;


Remove this property.

lbalmaceda · 2017-11-28T19:32:22Z

src/main/java/com/auth0/json/mgmt/ResourceServer.java

+        return isSystem;
+    }
+
+    public void setIsSystem(Boolean system) {


Remove setter as I'm not sure this works. It's easier to add it later 👍

lbalmaceda · 2017-11-28T19:34:36Z

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

+     * @param resourceServerId {@link ResourceServer#id} field
+     * @return request to execute
+     */
+    public Request<ResourceServer> get(String resourceServerId) {


All these entity methods that have a resourceServerId as a parameter, I'm checking the API docs and it says that it could be either the id or the identifier (audience) value. Assert this is true and in that case, rename to resourceServerIdOrIdentifier and explain this in the javadoc.

mfarsikov · 2017-11-29T14:03:15Z

Regarding testing setters: I would not recommend to create "cargo cult" around test-coverage.

I not sure but it should be possible to exclude data-classes from coverage testing.

Thank you for your patience :)

lbalmaceda

I agree with your comment about the test coverage but in this case, the serialization is not being tested completely neither in the client not in the pojo tests. I decided to make simpler the tests on the client by just checking the body size (in parameters count) and test the serialization in a separate pojo test, because jackson uses annotations to handle this. I've left you some final comments. Thanks

lbalmaceda · 2017-11-29T14:13:40Z

src/main/java/com/auth0/json/mgmt/ResourceServer.java

    private String name;
    @JsonProperty("identifier")
    private String identifier;
    @JsonProperty("scopes")
    private List<Scope> scopes;
-    @JsonProperty("signing_alg")
-    private String signingAlg;
+    @JsonProperty("signing_algorithm")


the json property was correct signing_alg, what changes is the pojo field name (the line below)

lbalmaceda · 2017-11-29T14:18:30Z

src/main/java/com/auth0/json/mgmt/ResourceServer.java

    public void setScopes(List<Scope> scopes) {
        this.scopes = scopes;
    }

-    public String getSigningAlg() {
-        return signingAlg;
+    @JsonProperty("signing_algorithm")


Remember to also fix this json property name.

lbalmaceda · 2017-11-29T14:18:50Z

src/main/java/com/auth0/json/mgmt/ResourceServer.java

    }

-    public void setSigningAlg(String signingAlg) {
-        this.signingAlg = signingAlg;
+    @JsonProperty("signing_algorithm")


And this one too

lbalmaceda · 2017-11-29T14:41:46Z

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

@@ -42,22 +42,18 @@ public void shouldListResourceServers() throws Exception {

    @Test
    public void shouldUpdateResourceServer() throws Exception {
+        ResourceServer resourceServer = new ResourceServer("https://api.my-company.com/api/v2/");
+        resourceServer.setId("23445566abab");


Just keep the new instance line, the pojo class serialization should be tested in a separate file as we did for the rest of them. The scope description serialization for example is never tested. Check the test/com/auth0/json/mgmt folder for examples and please add one class for each pojo. You can remove the call to the setters on this "shouldUpdateResourceServer" test.

mfarsikov · 2017-12-06T20:55:32Z

BTW: serialization test revealed a bug 👍

mfarsikov · 2017-12-06T20:56:57Z

Please let me know if this time it is ok - I will squash commits

lbalmaceda · 2017-12-07T15:55:50Z

@mfarsikov Thanks for the contribution. I'll prepare a minor release with these changes. 🎉

lbalmaceda suggested changes Nov 9, 2017

View reviewed changes

mfarsikov force-pushed the resource-server branch 2 times, most recently from 86e8997 to 3a3b9a7 Compare November 24, 2017 19:45

mfarsikov force-pushed the resource-server branch from 3a3b9a7 to 801209c Compare November 27, 2017 20:58

lbalmaceda suggested changes Nov 28, 2017

View reviewed changes

lbalmaceda reviewed Nov 28, 2017

View reviewed changes

lbalmaceda suggested changes Nov 29, 2017

View reviewed changes

mfarsikov added 4 commits December 6, 2017 21:25

Added CRUD for Resource server

1bac8b0

Another code review iteration

2334811

Another code review iteration

32d1bab

Serialization tests

d1d09cd

mfarsikov force-pushed the resource-server branch from 4cbb7cf to d1d09cd Compare December 6, 2017 20:52

lbalmaceda approved these changes Dec 7, 2017

View reviewed changes

lbalmaceda merged commit 424f99c into auth0:master Dec 7, 2017

lbalmaceda added the CH: Added label Dec 7, 2017

lbalmaceda modified the milestones: v1-Next, 1.5.0 Dec 7, 2017

mfarsikov deleted the resource-server branch December 7, 2017 16:16

Resource server #77

Resource server #77

Conversation

mfarsikov commented Sep 12, 2017

lbalmaceda left a comment • edited Loading

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

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 Nov 27, 2017

mfarsikov commented Nov 27, 2017

lbalmaceda left a comment • edited Loading

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

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

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

mfarsikov commented Nov 29, 2017

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

mfarsikov commented Dec 6, 2017

mfarsikov commented Dec 6, 2017

lbalmaceda commented Dec 7, 2017

lbalmaceda left a comment •

edited

Loading

lbalmaceda left a comment •

edited

Loading