move api/v2/version endpoint to OPTIONS /api #8506

bartcharbon · 2019-05-21T13:24:49Z

Checklist

Functionality works & meets specifications
Code reviewed
Code unit/integration/system tested
[n.a.] Migration step added in case of breaking change
[n.a.] User documentation updated
(If you have changed REST API interface) view-swagger.ftl updated
[n.a.] Test plan template updated
Clean commits
Added Feature/Fix to release notes

codecov · 2019-05-21T13:37:19Z

Codecov Report

Merging #8506 into master will increase coverage by <.1%.
The diff coverage is 85.7%.

@@            Coverage Diff             @@
##             master   #8506     +/-   ##
==========================================
+ Coverage      62.4%   62.4%   +<.1%     
- Complexity     8122    8130      +8     
==========================================
  Files          1316    1319      +3     
  Lines         38638   38651     +13     
  Branches       3865    3865             
==========================================
+ Hits          24119   24131     +12     
- Misses        12829   12830      +1     
  Partials       1690    1690

Flag	Coverage Δ	Complexity Δ
#api	`100% <ø> (ø)`	`0 <ø> (ø)`	⬇️
#integration	`7.6% <ø> (ø)`	`3 <ø> (ø)`	⬇️
#unit	`62.4% <85.7%> (ø)`	`8127 <6> (+8)`	⬆️

Impacted Files	Coverage Δ	Complexity Δ
...ava/org/molgenis/api/data/v2/RestControllerV2.java	`75.9% <ø> (ø)`	`71 <0> (ø)`	⬇️
...rc/main/java/org/molgenis/api/OptionsResponse.java	`100% <100%> (ø)`	`2 <2> (?)`
...main/java/org/molgenis/api/AppVersionResponse.java	`100% <100%> (ø)`	`2 <2> (?)`
.../src/main/java/org/molgenis/api/ApiController.java	`100% <100%> (ø)`	`7 <0> (+1)`	⬆️
.../main/java/org/molgenis/api/RootApiController.java	`77.7% <77.7%> (ø)`	`2 <2> (?)`
...ain/java/org/molgenis/data/importer/ImportRun.java	`86.6% <0%> (+2.2%)`	`23% <0%> (+1%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0c33a08...b7cf473. Read the comment docs.

molgenis-api-data/src/main/java/org/molgenis/api/data/v2/RestControllerV2.java

dennishendriksen · 2019-05-23T07:42:34Z

molgenis-api-data/src/test/java/org/molgenis/api/data/v2/RestControllerV2Test.java

@@ -55,6 +55,7 @@
 import static org.testng.reporters.Files.readFile;

 import com.google.common.collect.Sets;
+import java.text.ParseException;


Is suggest not to change any tests (see comment in RestControllerV2).

dennishendriksen · 2019-05-23T07:43:31Z

molgenis-api/src/main/java/org/molgenis/api/ApiController.java

@@ -17,7 +17,7 @@
   */
  public ApiController(String apiId, Integer apiVersion) {
    requireNonNull(apiId);
-    if (!API_ID_PATTERN.matcher(apiId).matches()) {
+    if (!apiId.isEmpty() && !API_ID_PATTERN.matcher(apiId).matches()) {


Add a unit test for empty apiId

dennishendriksen · 2019-05-23T07:44:47Z

molgenis-api/src/main/java/org/molgenis/api/VersionApiNamespace.java

@@ -0,0 +1,8 @@
+package org.molgenis.api;
+
+public class VersionApiNamespace {


We agreed on OPTIONS /api to expose the application version, right? That would make this class unnecessary.

dennishendriksen · 2019-05-23T07:45:53Z

molgenis-api/src/main/java/org/molgenis/api/VersionController.java

+
+@RestController
+@RequestMapping(API_PATH)
+public class VersionController extends ApiController {


How about renaming this class to RootApiController so we can extend it with other planned endpoints such as listing all apis with their versions?

dennishendriksen · 2019-05-23T07:49:48Z

molgenis-api/src/main/java/org/molgenis/api/VersionController.java

+
+  @Autowired
+  @RequestMapping(method = OPTIONS)
+  public VersionResponse getVersion() throws ParseException {


I suggest storing the version information under the app key.

{ "app": { "version": "...", "buildDate": "..." }, "apis": [] }

The apis key could be added in the future.

dennishendriksen · 2019-05-23T07:52:39Z

molgenis-api/src/test/java/org/molgenis/api/VersionControllerTest.java

+            .andReturn();
+    String actual = result.getResponse().getContentAsString();
+    String expected =
+        "{\"molgenisVersion\":\"versionString\",\"buildDate\":\"2019-05-21T10:32:00Z\"}";


I think that it is a good idea to use one format for date-times in JSON requests/responses everywhere. Did you look into adding milliseconds? It would make sense considering the places where we use datetimes.

Response object contains an Instant, the conversion to json is than done centrally by the application right?

dennishendriksen · 2019-05-23T07:53:06Z

molgenis-api/src/main/java/org/molgenis/api/VersionResponse.java

+@AutoGson(autoValueClass = AutoValue_VersionResponse.class)
+@SuppressWarnings("squid:S1610")
+public abstract class VersionResponse {
+  public abstract String getMolgenisVersion();


getVersion instead of getMolgenisVersion

dennishendriksen · 2019-05-23T07:54:03Z

molgenis-api/src/test/java/org/molgenis/api/VersionControllerTest.java

+        new VersionController("versionString", "2019-05-21 10:32 UTC");
+    mockMvc =
+        MockMvcBuilders.standaloneSetup(versionController)
+            .setMessageConverters(new FormHttpMessageConverter(), gsonHttpMessageConverter)


I suspect you don't need a FormHttpMessageConverter here.

dennishendriksen · 2019-05-24T05:31:53Z

molgenis-api/src/test/java/org/molgenis/api/VersionControllerTest.java

+@TestExecutionListeners(listeners = WithSecurityContextTestExecutionListener.class)
+@TestPropertySource(properties = {"molgenis.version = 10.3.8"})
+@ContextConfiguration(classes = {GsonConfig.class})
+public class VersionControllerTest extends AbstractTestNGSpringContextTests {


Rename to RootApiControllerTest

dennishendriksen · 2019-05-24T05:34:37Z

molgenis-api/src/main/java/org/molgenis/api/RootApiController.java

+
+@RestController
+@RequestMapping(API_PATH)
+public class RootApiController extends ApiController {


Add swagger annotations

swagger not working yet

dennishendriksen · 2019-05-24T05:35:30Z

molgenis-api/src/main/java/org/molgenis/api/VersionResponse.java

+@AutoValue
+@AutoGson(autoValueClass = AutoValue_VersionResponse.class)
+@SuppressWarnings("squid:S1610")
+public abstract class VersionResponse {


Rename to something like OptionsResponse/ApiOptionsResponse

dennishendriksen · 2019-05-24T05:36:15Z

molgenis-api/src/main/java/org/molgenis/api/RootApiController.java

+
+  @Autowired
+  @RequestMapping(method = OPTIONS)
+  public VersionResponse getVersion() throws ParseException {


Deprecate /api/v2 version endpoint and refer to this endpoint

bartcharbon added the branch:master label May 21, 2019

dennishendriksen requested changes May 23, 2019

View reviewed changes

bartcharbon force-pushed the api/version branch 2 times, most recently from ce6268f to 681e992 Compare May 23, 2019 12:34

move api/v2/version endpoint to OPTIONS /api

c19f272

dennishendriksen requested changes May 24, 2019

View reviewed changes

bartcharbon added 2 commits May 24, 2019 08:18

Add deprication, swagger annotations and rename test class

8817b66

deprecate copy endpoint

e960e34

bartcharbon force-pushed the api/version branch from 681e992 to e960e34 Compare May 24, 2019 06:53

bartcharbon added 2 commits May 24, 2019 10:56

rename class

b82d849

add swagger

b7cf473

dennishendriksen approved these changes May 24, 2019

View reviewed changes

dennishendriksen merged commit 5138856 into molgenis:master May 24, 2019

bartcharbon deleted the api/version branch August 27, 2019 09:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

move api/v2/version endpoint to OPTIONS /api #8506

move api/v2/version endpoint to OPTIONS /api #8506

bartcharbon commented May 21, 2019 •

edited by dennishendriksen

codecov bot commented May 21, 2019 •

edited

dennishendriksen May 23, 2019

dennishendriksen May 23, 2019

dennishendriksen May 23, 2019

dennishendriksen May 23, 2019

dennishendriksen May 23, 2019

dennishendriksen May 23, 2019

bartcharbon May 23, 2019

dennishendriksen May 23, 2019

dennishendriksen May 23, 2019

dennishendriksen May 24, 2019

dennishendriksen May 24, 2019

dennishendriksen May 24, 2019

dennishendriksen May 24, 2019

dennishendriksen May 24, 2019

		@@ -0,0 +1,8 @@
		package org.molgenis.api;

		public class VersionApiNamespace {

move api/v2/version endpoint to OPTIONS /api #8506

move api/v2/version endpoint to OPTIONS /api #8506

Conversation

bartcharbon commented May 21, 2019 • edited by dennishendriksen

Checklist

codecov bot commented May 21, 2019 • edited

Codecov Report

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

bartcharbon commented May 21, 2019 •

edited by dennishendriksen

codecov bot commented May 21, 2019 •

edited