Repository: eclipse/microprofile-open-api
Pull request #348 (2019-05-17 05:38:05)

Remove ServerVariables
Issue: #245
This is a follow-up of PRs: #317, #321, #330

---

With `1.1` we have indicated that `ServerVariables` will be removed with `2.0` and we have told our user they should use `Map<String, ServerVariable>` instead of `ServerVariables`. Back then `ServerVariables` was a subtype of `Map<String, ServerVariable>`

Currently with `2.0-SNAPSHOT`, `ServerVariables` is no longer a subtype of Map<String, ServerVariable>`, this will break user code that have followed our advice and that have started to use `Map<String, ServerVariable>`.

This PR is doing the rest of the work as planned in #245, removing `ServerVariables` completely, (this is similar to what we just did in #345 for `Scope`).

---
cc @EricWittmann, @arthurdm 


Patch:
diff --git a/api/src/main/java/org/eclipse/microprofile/openapi/OASFactory.java b/api/src/main/java/org/eclipse/microprofile/openapi/OASFactory.java
index 24cfe40..eb5f1f3 100644
--- a/api/src/main/java/org/eclipse/microprofile/openapi/OASFactory.java
+++ b/api/src/main/java/org/eclipse/microprofile/openapi/OASFactory.java
@@ -46,7 +46,6 @@ import org.eclipse.microprofile.openapi.models.security.SecurityRequirement;
 import org.eclipse.microprofile.openapi.models.security.SecurityScheme;
 import org.eclipse.microprofile.openapi.models.servers.Server;
 import org.eclipse.microprofile.openapi.models.servers.ServerVariable;
-import org.eclipse.microprofile.openapi.models.servers.ServerVariables;
 import org.eclipse.microprofile.openapi.models.tags.Tag;
 import org.eclipse.microprofile.openapi.spi.OASFactoryResolver;
 
@@ -341,17 +340,6 @@ public final class OASFactory {
         return createObject(ServerVariable.class);
     }
 
-    /**
-     * This method creates a new {@link org.eclipse.microprofile.openapi.models.servers.ServerVariables} instance.
-     *
-     * @return a new ServerVariables instance
-     * @deprecated since 1.1, use <code>Map&lt;String, ServerVariable&gt;</code> for server variables instead
-     */
-    @Deprecated
-    public static ServerVariables createServerVariables() {
-        return createObject(ServerVariables.class);
-    }
-
     /**
      * This method creates a new {@link org.eclipse.microprofile.openapi.models.tags.Tag} instance.
      *
diff --git a/api/src/main/java/org/eclipse/microprofile/openapi/models/servers/Server.java b/api/src/main/java/org/eclipse/microprofile/openapi/models/servers/Server.java
index 99df2ca..d4da614 100644
--- a/api/src/main/java/org/eclipse/microprofile/openapi/models/servers/Server.java
+++ b/api/src/main/java/org/eclipse/microprofile/openapi/models/servers/Server.java
@@ -109,21 +109,26 @@ public interface Server extends Constructible, Extensible<Server> {
      * <p>
      * Variables are represented as a map between variable name and its value. The value is used for substitution in the server's URL template.
      * </p> 
-     * @return ServerVariables variables (in the future this method will instead return a <code>Map&lt;String, ServerVariable&gt;</code>).
+     * @return map of server variables (potentially immutable).
      **/
-    ServerVariables getVariables();
+    Map<String, ServerVariable> getVariables();
 
     /**
-     * This method sets the variables property of Server instance to the given variables argument.
-     * <p>
-     * Variables property is a map between variable name and its value. The value is used for substitution in the server's URL template.
-     * </p>
-     * @param variables a map between variable name and its value
-     * @deprecated since 1.1, use {@link #setVariables(Map)} instead
+     * Adds the given server variable item to the Server's map of variables.
+     *
+     * @param variableName the name the variable to add
+     * @param variable a server variable used for substitution in the server's URL template.
+     * @return the current Server object
+     **/
+    public Server addVariable(String variableName, ServerVariable variable);
+
+    /**
+     * Removes the given server variable item from the Server's map of variables
+     * 
+     * @param variableName the name the variable to remove
      */
-    @Deprecated
-    void setVariables(ServerVariables variables);
-    
+    public void removeVariable(String variableName);
+
     /**
      * This method sets the variables property of Server instance to the given variables argument.
      * <p>
@@ -132,21 +137,6 @@ public interface Server extends Constructible, Extensible<Server> {
      * @param variables a map between variable name and its value
      */
     void setVariables(Map<String, ServerVariable> variables);
-    
-    /**
-     * This method sets the variables property of Server instance to the given variables argument and returns the modified instance.
-     * <p>
-     * Variables property is a map between variable name and its value. The value is used for substitution in the server's URL template.
-     * </p>
-     * @param variables a map between variable name and its value
-     * @return Server instance with the set variables property.
-     * @deprecated since 1.1, use {@link #variables(Map)} instead
-     */
-    @Deprecated
-    default Server variables(ServerVariables variables) {
-        setVariables(variables);
-        return this;
-    }
 
     /**
      * This method sets the variables property of Server instance to the given variables argument and returns the modified instance.
diff --git a/api/src/main/java/org/eclipse/microprofile/openapi/models/servers/ServerVariables.java b/api/src/main/java/org/eclipse/microprofile/openapi/models/servers/ServerVariables.java
deleted file mode 100644
index ccfe9c9..0000000
--- a/api/src/main/java/org/eclipse/microprofile/openapi/models/servers/ServerVariables.java
+++ /dev/null
@@ -1,93 +0,0 @@
-/**
- * Copyright (c) 2017 Contributors to the Eclipse Foundation
- * Copyright 2017 SmartBear Software
- * <p>
- * Licensed 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
- * <p>
- * http://www.apache.org/licenses/LICENSE-2.0
- * <p>
- * 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.eclipse.microprofile.openapi.models.servers;
-
-import java.util.Map;
-
-import org.eclipse.microprofile.openapi.models.Constructible;
-import org.eclipse.microprofile.openapi.models.Extensible;
-
-/**
- * ServerVariables
- *
- * @see <a href="https://github.com/OAI/OpenAPI-Specification/blob/master/versions/3.0.0.md#server-variable-object">ServerVariable Object</a>
- * @deprecated since 1.1, a future version will remove this and replace it with a <code>Map&lt;String, ServerVariable&gt;</code>, because it does not
- * need to be extensible
- */
-@Deprecated
-public interface ServerVariables extends Constructible, Extensible<ServerVariables> {
-
-    /**
-     * This method adds a key-value item to a ServerVariables instance from the name-item parameter pair and returns the modified instance.
-     *
-     * @param name the name of ServerVariable instance
-     * @param serverVariable the ServerVariable instance. null values will be rejected (implementation will throw an exception) or ignored.
-     * @return ServerVariables instance with the added name-item pair.
-     */
-    ServerVariables addServerVariable(String name, ServerVariable serverVariable);
-
-    /**
-     * Removes the given server variables.
-     * 
-     * @param name the name of ServerVariable instance
-     */
-    void removeServerVariable(String name);
-
-    /**
-     * Returns a copy map (potentially immutable) of the server variables.
-     * 
-     * @return all items
-     */
-    Map<String, ServerVariable> getServerVariables();
-
-    /**
-     * Set the server variables map to this ServerVariables object.
-     * 
-     * @param items a map containing key-value item.
-     */
-    void setServerVariables(Map<String, ServerVariable> items);
-
-    /**
-     * Check whether a server variable is present in the map. This is a convenience method for <code>getServerVariables().containsKey(name)</code>
-     * 
-     * @param name the name of ServerVariable instance
-     * @return a boolean to indicate if the server variable is present or not.
-     */
-    default boolean hasServerVariable(String name) {
-        Map<String, ServerVariable> map = getServerVariables();
-        if (map == null) {
-            return false;
-        }
-        return map.containsKey(name);
-    }
-
-    /**
-     * Returns a server variable for a given name. This is a convenience method for <code>getServerVariables().get(name)</code>
-     * 
-     * @param name the name of ServerVariable instance
-     * @return the corresponding server variable or null.
-     */
-    default ServerVariable getServerVariable(String name) {
-        Map<String, ServerVariable> map = getServerVariables();
-        if (map == null) {
-            return null;
-        }
-        return map.get(name);
-    }
-
-}
\ No newline at end of file
diff --git a/api/src/main/java/org/eclipse/microprofile/openapi/models/servers/package-info.java b/api/src/main/java/org/eclipse/microprofile/openapi/models/servers/package-info.java
index c622765..ce56736 100644
--- a/api/src/main/java/org/eclipse/microprofile/openapi/models/servers/package-info.java
+++ b/api/src/main/java/org/eclipse/microprofile/openapi/models/servers/package-info.java
@@ -22,18 +22,16 @@
  * <pre>
  * {@literal}Override
  *  public OpenAPI buildModel() {
- *      return OASFactory.createObject(OpenAPI.class)
- *          .servers(new ArrayList&lt;Server&gt;())
- *              .addServer(OASFactory.createObject(Server.class)
- *                  .url("https://{username}.gigantic-server.com:{port}/{basePath}")
- *                  .description("The production API server")
- *                  .variables(OASFactory.createObject(ServerVariables.class)
- *                      .addServerVariable("username", OASFactory.createObject(ServerVariable.class)
- *                          .defaultValue("user1")
- *                          .description("Reviews of the app by users")
- *                          .enumeration(new ArrayList&lt;String&gt;())
+ *      return OASFactory.createOpenAPI()
+ *              .addServer(OASFactory.createServer()
+ *                      .url("https://{username}.example-server.com:8080/api")
+ *                      .description("The production API server")
+ *                      .addVariable("username", OASFactory.createServerVariable()
+ *                              .defaultValue("user1")
+ *                              .description("Reviews of the app by users")
  *                              .addEnumeration("user1")
- *                              .addEnumeration("user2")))
+ *                              .addEnumeration("user2")));
+ *  }
  * </pre>
  */
 
diff --git a/tck/src/main/java/org/eclipse/microprofile/openapi/reader/MyOASModelReaderImpl.java b/tck/src/main/java/org/eclipse/microprofile/openapi/reader/MyOASModelReaderImpl.java
index 34cb0fa..efd1a87 100644
--- a/tck/src/main/java/org/eclipse/microprofile/openapi/reader/MyOASModelReaderImpl.java
+++ b/tck/src/main/java/org/eclipse/microprofile/openapi/reader/MyOASModelReaderImpl.java
@@ -45,7 +45,6 @@ import org.eclipse.microprofile.openapi.models.security.SecurityRequirement;
 import org.eclipse.microprofile.openapi.models.security.SecurityScheme;
 import org.eclipse.microprofile.openapi.models.servers.Server;
 import org.eclipse.microprofile.openapi.models.servers.ServerVariable;
-import org.eclipse.microprofile.openapi.models.servers.ServerVariables;
 import org.eclipse.microprofile.openapi.models.tags.Tag;
 
 
@@ -72,21 +71,20 @@ public class MyOASModelReaderImpl implements OASModelReader {
                     .addServer(OASFactory.createObject(Server.class)
                         .url("https://{username}.gigantic-server.com:{port}/{basePath}")
                         .description("The production API server")
-                        .variables(OASFactory.createObject(ServerVariables.class)
-                            .addServerVariable("username", OASFactory.createObject(ServerVariable.class)
-                                .defaultValue("user1")
-                                .description("Reviews of the app by users")
-                                .enumeration(new ArrayList<String>())
-                                    .addEnumeration("user1")
-                                    .addEnumeration("user2"))
-                            .addServerVariable("port", OASFactory.createObject(ServerVariable.class)
-                                .defaultValue("8443")
-                                .description("Booking data"))
-                            .addServerVariable("user", OASFactory.createObject(ServerVariable.class)
-                                .defaultValue("user")
-                                .description("User data"))
-                            .addServerVariable("basePath", OASFactory.createObject(ServerVariable.class)
-                                .defaultValue("v2"))))
+                        .addVariable("username", OASFactory.createObject(ServerVariable.class)
+                            .defaultValue("user1")
+                            .description("Reviews of the app by users")
+                            .enumeration(new ArrayList<String>())
+                                .addEnumeration("user1")
+                                .addEnumeration("user2"))
+                        .addVariable("port", OASFactory.createObject(ServerVariable.class)
+                            .defaultValue("8443")
+                            .description("Booking data"))
+                        .addVariable("user", OASFactory.createObject(ServerVariable.class)
+                            .defaultValue("user")
+                            .description("User data"))
+                        .addVariable("basePath", OASFactory.createObject(ServerVariable.class)
+                            .defaultValue("v2")))
                     .addServer(OASFactory.createObject(Server.class)
                         .url("https://test-server.com:80/basePath")
                         .description("The test API server"))


Test patch:
diff --git a/tck/src/main/java/org/eclipse/microprofile/openapi/tck/ModelConstructionTest.java b/tck/src/main/java/org/eclipse/microprofile/openapi/tck/ModelConstructionTest.java
index 8f06e23..5ab7344 100644
--- a/tck/src/main/java/org/eclipse/microprofile/openapi/tck/ModelConstructionTest.java
+++ b/tck/src/main/java/org/eclipse/microprofile/openapi/tck/ModelConstructionTest.java
@@ -73,13 +73,12 @@ import org.eclipse.microprofile.openapi.models.security.SecurityRequirement;
 import org.eclipse.microprofile.openapi.models.security.SecurityScheme;
 import org.eclipse.microprofile.openapi.models.servers.Server;
 import org.eclipse.microprofile.openapi.models.servers.ServerVariable;
-import org.eclipse.microprofile.openapi.models.servers.ServerVariables;
 import org.eclipse.microprofile.openapi.models.tags.Tag;
-import org.testng.annotations.Test;
 import org.jboss.arquillian.container.test.api.Deployment;
 import org.jboss.arquillian.testng.Arquillian;
 import org.jboss.shrinkwrap.api.ShrinkWrap;
 import org.jboss.shrinkwrap.api.spec.WebArchive;
+import org.testng.annotations.Test;
 
 /**
  * This test covers construction of the OpenAPI model. It verifies that the implementation can
@@ -1231,10 +1230,16 @@ public class ModelConstructionTest extends Arquillian {
         
         final ServerVariable sv1 = createConstructibleInstance(ServerVariable.class);
         server.setVariables(Collections.singletonMap("var1", sv1));
-        Map<String,ServerVariable> variables = server.getVariables().getServerVariables();
+        Map<String,ServerVariable> variables = server.getVariables();
         assertEquals(variables.size(), 1, "The map is expected to contain one entry.");
         assertTrue(variables.containsKey("var1"), "The map is expected to contain a 'var1' entry.");
         assertEquals(variables.get("var1"), sv1, "The value corresponding to the 'var1' is wrong.");
+        checkMapEntry(server.getVariables(), "var1", sv1);
+        
+        final ServerVariable sv2 = createConstructibleInstance(ServerVariable.class);
+        checkMapImmutable(server, Server::getVariables, "sv2", sv2);
+        final ServerVariable sv3 = createConstructibleInstance(ServerVariable.class);
+        checkNullValueInAdd(server::getVariables, server::addVariable, "sv3", sv3);
         
         server.setVariables((Map<String, ServerVariable>) null);
         assertNull(server.getVariables(), "The value is expected to be null.");
@@ -1263,43 +1268,6 @@ public class ModelConstructionTest extends Arquillian {
         checkListImmutable(sv, ServerVariable::getEnumeration , otherEnumerationValue);
     }
     
-    @Test
-    @Deprecated
-    public void serverVariablesTest() {
-        final ServerVariables svs = processConstructible(ServerVariables.class);
-        
-        final String varKey = "myServerVariable";
-        final ServerVariable varValue = createConstructibleInstance(ServerVariable.class);
-        svs.setServerVariables(Collections.singletonMap(varKey, varValue));
-        assertTrue(svs.hasServerVariable(varKey), varKey + " is present in the map");
-        assertEquals(svs.getServerVariables().size(), 1, "The map is expected to contain one entry.");
-        assertSame(svs.getServerVariable(varKey), varValue, 
-                "The value associated with the key: " + varKey + " is expected to be the same one that was added.");
-        checkMapEntry(svs.getServerVariables(), varKey, varValue);
-        
-        final String varKey2 = "myServerVariable2";
-        assertFalse(svs.hasServerVariable(varKey2), varKey2 + " is absent in the map");
-        final ServerVariable varValue2 = createConstructibleInstance(ServerVariable.class);
-        checkSameObject(svs, svs.addServerVariable(varKey2, varValue2));
-        assertTrue(svs.hasServerVariable(varKey2), varKey2 + " is present in the map");
-        assertEquals(svs.getServerVariables().size(), 2, "The map is expected to contain two entries.");
-        assertSame(svs.getServerVariable(varKey2), varValue2, 
-                "The value associated with the key: " + varKey2 + " is expected to be the same one that was added.");
-        checkMapEntry(svs.getServerVariables(), varKey2, varValue2);
-        
-        svs.removeServerVariable(varKey);
-        assertFalse(svs.hasServerVariable(varKey), varKey + " is absent in the map");
-        assertEquals(svs.getServerVariables().size(), 1, "The map is expected to contain one entry.");
-        
-        svs.removeServerVariable(varKey2);
-        assertFalse(svs.hasServerVariable(varKey2), varKey + " is absent in the map");
-        assertEquals(svs.getServerVariables().size(), 0, "The map is expected to contain 0 entries.");
-        
-        final ServerVariable otherValue = createConstructibleInstance(ServerVariable.class);
-        checkMapImmutable(svs, ServerVariables::getServerVariables, "otherServerVariable", otherValue);
-        checkNullValueInAdd(svs::getServerVariables, svs::addServerVariable, "other", otherValue);
-    }
-    
     @Test
     public void tagTest() {
         processConstructible(Tag.class);