[GitHub] [syncope] ilgrosso commented on a change in pull request #207: SYNCOPE-1580: Manage WA Configuration Properties/Schema

[GitBox <gi...@apache.org>]

ilgrosso commented on a change in pull request #207:
URL: https://github.com/apache/syncope/pull/207#discussion_r457931542



##########
File path: core/persistence-api/src/main/java/org/apache/syncope/core/persistence/api/entity/auth/WAConfigEntry.java
##########
@@ -0,0 +1,34 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.syncope.core.persistence.api.entity.auth;
+
+import org.apache.syncope.core.persistence.api.entity.ProvidedKeyEntity;
+
+import java.io.Serializable;
+
+public interface WAConfigEntry<T extends Serializable> extends ProvidedKeyEntity {
+
+    String getName();

Review comment:
       What is the need for `name`? Since `key` is to be provided (not generated, correctly), `name` seems to be just a duplicate, no?

##########
File path: core/persistence-jpa/src/main/java/org/apache/syncope/core/persistence/jpa/entity/auth/JPAWAConfigEntry.java
##########
@@ -0,0 +1,64 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.syncope.core.persistence.jpa.entity.auth;
+
+import org.apache.syncope.core.persistence.api.entity.auth.WAConfigEntry;
+import org.apache.syncope.core.persistence.jpa.entity.AbstractGeneratedKeyEntity;
+
+import javax.persistence.Column;
+import javax.persistence.Entity;
+import javax.persistence.Lob;
+import javax.persistence.Table;
+
+import java.io.Serializable;
+
+@Entity
+@Table(name = JPAWAConfigEntry.TABLE)
+public class JPAWAConfigEntry extends AbstractGeneratedKeyEntity implements WAConfigEntry {

Review comment:
       Since `WAConfigEntry` extends `ProvidedKeyEntity`, this class must extend `AbstractProvidedKeyEntity`.
   
   As said above, `name` should be removed.

##########
File path: fit/core-reference/src/test/java/org/apache/syncope/fit/core/WAConfigITCase.java
##########
@@ -0,0 +1,64 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.syncope.fit.core;
+
+import org.apache.syncope.common.lib.SyncopeClientException;
+import org.apache.syncope.common.lib.to.WAConfigTO;
+import org.apache.syncope.common.rest.api.RESTHeaders;
+import org.apache.syncope.fit.AbstractITCase;
+import org.junit.jupiter.api.Test;
+
+import javax.ws.rs.core.Response;
+
+import java.util.UUID;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+
+public class WAConfigITCase extends AbstractITCase {
+    @Test
+    public void verify() {
+        WAConfigTO configTO = new WAConfigTO.Builder()

Review comment:
       Please add tests for more value types.

##########
File path: core/persistence-jpa/src/test/java/org/apache/syncope/core/persistence/jpa/inner/WAConfigTest.java
##########
@@ -0,0 +1,78 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.syncope.core.persistence.jpa.inner;
+
+import org.apache.syncope.core.persistence.api.dao.auth.WAConfigDAO;
+import org.apache.syncope.core.persistence.api.entity.auth.WAConfigEntry;
+import org.apache.syncope.core.persistence.jpa.AbstractTest;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.transaction.annotation.Transactional;
+
+import java.io.Serializable;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+
+@Transactional("Master")
+public class WAConfigTest extends AbstractTest {
+
+    @Autowired
+    private WAConfigDAO configDAO;
+    
+    @BeforeEach
+    public void beforeEach() {
+        configDAO.deleteAll();
+    }
+
+    @Test
+    public void save() {
+        create("system.example.key[0]", "value1,value2,value3");
+        assertFalse(configDAO.findAll().isEmpty());
+    }
+
+    @Test
+    public void update() {
+        WAConfigEntry entry = create("system.syncope.key[0]", "value1,value2,value3");

Review comment:
       Please add tests for more value types.

##########
File path: common/am/rest-api/src/main/java/org/apache/syncope/common/rest/api/service/wa/WAConfigService.java
##########
@@ -0,0 +1,114 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.syncope.common.rest.api.service.wa;
+
+import io.swagger.v3.oas.annotations.Parameter;
+import io.swagger.v3.oas.annotations.enums.ParameterIn;
+import io.swagger.v3.oas.annotations.headers.Header;
+import io.swagger.v3.oas.annotations.media.Schema;
+import io.swagger.v3.oas.annotations.responses.ApiResponse;
+import io.swagger.v3.oas.annotations.responses.ApiResponses;
+import io.swagger.v3.oas.annotations.security.SecurityRequirement;
+import io.swagger.v3.oas.annotations.security.SecurityRequirements;
+import io.swagger.v3.oas.annotations.tags.Tag;
+import org.apache.syncope.common.lib.to.WAConfigTO;
+import org.apache.syncope.common.rest.api.RESTHeaders;
+import org.apache.syncope.common.rest.api.service.JAXRSService;
+
+import javax.validation.constraints.NotNull;
+import javax.ws.rs.Consumes;
+import javax.ws.rs.DELETE;
+import javax.ws.rs.GET;
+import javax.ws.rs.POST;
+import javax.ws.rs.PUT;
+import javax.ws.rs.Path;
+import javax.ws.rs.PathParam;
+import javax.ws.rs.Produces;
+import javax.ws.rs.core.HttpHeaders;
+import javax.ws.rs.core.MediaType;
+import javax.ws.rs.core.Response;
+
+import java.util.List;
+
+/**
+ * REST operations for WA Configuration.
+ */
+@Tag(name = "WA")
+@SecurityRequirements({
+    @SecurityRequirement(name = "BasicAuthentication"),
+    @SecurityRequirement(name = "Bearer")})
+@Path("wa/config")
+public interface WAConfigService extends JAXRSService {
+
+    @GET
+    @Consumes({MediaType.APPLICATION_JSON, RESTHeaders.APPLICATION_YAML, MediaType.APPLICATION_XML})
+    @Produces({MediaType.APPLICATION_JSON, RESTHeaders.APPLICATION_YAML, MediaType.APPLICATION_XML})
+    List<WAConfigTO> list();
+
+    @GET
+    @Path("{key}")
+    @Consumes({MediaType.APPLICATION_JSON, RESTHeaders.APPLICATION_YAML, MediaType.APPLICATION_XML})
+    @Produces({MediaType.APPLICATION_JSON, RESTHeaders.APPLICATION_YAML, MediaType.APPLICATION_XML})
+    WAConfigTO read(@NotNull @PathParam("key") String key);
+
+    @GET
+    @Path("byName/{name}")
+    @Consumes({MediaType.APPLICATION_JSON, RESTHeaders.APPLICATION_YAML, MediaType.APPLICATION_XML})
+    @Produces({MediaType.APPLICATION_JSON, RESTHeaders.APPLICATION_YAML, MediaType.APPLICATION_XML})
+    WAConfigTO readByName(@NotNull @PathParam("name") String name);
+
+    @ApiResponses({
+        @ApiResponse(responseCode = "201",
+            description = "WAConfigTO successfully created", headers = {
+            @Header(name = RESTHeaders.RESOURCE_KEY, schema =
+            @Schema(type = "string"),
+                description = "UUID generated for the entity created"),
+            @Header(name = HttpHeaders.LOCATION, schema =
+            @Schema(type = "string"),
+                description = "URL of the entity created")}),
+        @ApiResponse(responseCode = "409",
+            description = "Config already existing")})
+    @POST
+    @Consumes({MediaType.APPLICATION_JSON, RESTHeaders.APPLICATION_YAML, MediaType.APPLICATION_XML})
+    @Produces({MediaType.APPLICATION_JSON, RESTHeaders.APPLICATION_YAML, MediaType.APPLICATION_XML})
+    Response create(@NotNull WAConfigTO configTO);
+
+    @Parameter(name = "key", description = "WAConfigTO's key", in = ParameterIn.PATH, schema =
+    @Schema(type = "string"))
+    @ApiResponses(
+        @ApiResponse(responseCode = "204", description = "Operation was successful"))
+    @PUT
+    @Path("{key}")
+    @Consumes({MediaType.APPLICATION_JSON, RESTHeaders.APPLICATION_YAML, MediaType.APPLICATION_XML})
+    @Produces({MediaType.APPLICATION_JSON, RESTHeaders.APPLICATION_YAML, MediaType.APPLICATION_XML})
+    void update(@NotNull WAConfigTO configTO);
+
+    @DELETE
+    @Consumes({MediaType.APPLICATION_JSON, RESTHeaders.APPLICATION_YAML, MediaType.APPLICATION_XML})
+    @Produces({MediaType.APPLICATION_JSON, RESTHeaders.APPLICATION_YAML, MediaType.APPLICATION_XML})
+    @Path("{key}")
+    Response delete(@NotNull @PathParam("key") String key);

Review comment:
       Please change return type from `Response` to `void`




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org