Implement applications endpoint #16

pathob · 2020-09-29T09:37:52Z

No description provided.

pathob

Hallo Cosmin,

sehr schön, die wichtigsten Methoden sind getestet und die Coverage von 80% ist erreicht. Inhaltlich ist nur ein Test falsch, über den sprechen wir nochmal zusammen, alle anderen Tests sind inhaltlich gut so, können und sollten aber entsprechend meiner Kommentare nochmal überarbeitet werden.

src/test/java/de/aservo/confapi/crowd/model/util/ApplicationBeanTest.java

src/test/java/de/aservo/confapi/crowd/service/ApplicaitonsServiceImplTest.java

src/main/java/de/aservo/confapi/crowd/model/ApplicationBean.java

src/main/java/de/aservo/confapi/crowd/rest/ApplicationsResourceImpl.java

src/test/java/de/aservo/confapi/crowd/model/util/ApplicationBeanTest.java

src/test/java/de/aservo/confapi/crowd/service/ApplicaitonsServiceImplTest.java

pathob

Weiß nicht, ob das neue Sachen sind oder ich die letztes mal übersehen habe...

src/test/java/de/aservo/confapi/crowd/model/util/ApplicationBeanTest.java

lehmannk · 2020-10-06T07:02:50Z

src/main/java/de/aservo/confapi/crowd/service/ApplicationsServiceImpl.java

+        try {
+            final ImmutableApplication.Builder applicationBuilder = new ImmutableApplication.Builder(applicationManager.findById(id));
+
+            if (applicationBean.getName() != null) {
+                applicationBuilder.setName(applicationBean.getName());
+            }
+            if (applicationBean.getType() != null) {
+                applicationBuilder.setType(ApplicationBeanUtil.toApplicationType(applicationBean.getType()));
+            }
+            if (applicationBean.getDescription() != null) {
+                applicationBuilder.setDescription(applicationBean.getDescription());
+            }
+            if (applicationBean.getActive() != null) {
+                applicationBuilder.setActive(applicationBean.getActive());
+            }
+            if (applicationBean.getPassword() != null) {
+                applicationBuilder.setPasswordCredential(PasswordCredential.unencrypted(applicationBean.getPassword()));
+            }


Siehe meine Fragestellung bzgl PUT und dem POLA principle. Nach einiger Überlegung halte ich es für eine bessere Idee, die props grundsätzlich immer zu übernehmen, auch wenn explizit null gewünscht ist.

Die Überlegung ist nicht verkehrt, aber das funktioniert so nicht mit dem JSON Framework, weil wir nicht unterscheiden können, ob der User Beispielsweise für die Description null als Wert übergeben hat oder die Description im Bean einfach weggelassen hat, weil sie sich nicht ändern soll.

Wie funktioniert denn die Atlassian REST API? Ist das mit dem Werten weglassen, wenn sie sich nicht ändern sollen, dort auch so oder muss man IMMER alles mitschicken?

Wenn es bei Atlassian auch so ist wie bei uns bisher, dann würde ich das so lassen. Für Type und Active macht null sowieso keinen Sinn, für Name, Description und Password kann man sonst auch einen leeren String schicken.

habe zb. mal PUT mit spaces (nativ REST API) ausprobiert und beim json den name key weggelassen. Das liefert:

"errors": [ { "message": { "translation": "Name is blank. Cannot update space to have no name.", "args": [] } } ]

d.h. null Werte werden nicht ignoriert. Wir sollten es daher auch nicht tun. Wie ich vermutet habe checkt das ATL backend ohnehin nochmal auf Plausi.

Okay, ich würde das gerne auch nochmal für Jira testen, aber guter Punkt, dann sollten wir das auch so machen. Hoffe uns fliegen dann nicht die Beans mit den Objekttypen statt primitiven Datentypen um die Ohren...

Bei Jira ist es nicht so.

Beispiel:

GET /rest/api/2/issuetype/10001

Response:

{ "self": "http://localhost:2990/jira/rest/api/2/issuetype/10001", "id": "10001", "description": "Created by Jira Software - do not edit or delete. Issue type for a user story.", "iconUrl": "http://localhost:2990/jira/images/icons/issuetypes/story.svg", "name": "Story", "subtask": false }

PUT /rest/api/2/issuetype/10001

Request:

{ "name": "Story", "description": null }

oder

{ "name": "Story" }

Response:

{ "self": "http://localhost:2990/jira/rest/api/2/issuetype/10001", "id": "10001", "description": "Created by Jira Software - do not edit or delete. Issue type for a user story.", "iconUrl": "http://localhost:2990/jira/images/icons/issuetypes/story.svg", "name": "Story", "subtask": false }

...aber...

Request:

{ "name": "Story", "description": "" }

Response;

{ "self": "http://localhost:2990/jira/rest/api/2/issuetype/10001", "id": "10001", "description": "", "iconUrl": "http://localhost:2990/jira/images/icons/issuetypes/story.svg", "name": "Story", "subtask": false }

lehmannk · 2020-10-06T07:06:07Z

src/main/java/de/aservo/confapi/crowd/model/util/ApplicationBeanUtil.java

+        if (ApplicationType.GENERIC_APPLICATION.equals(applicationType)) {
+            return ApplicationBean.ApplicationType.GENERIC;
+        } else if (ApplicationType.PLUGIN.equals(applicationType)) {
+            return ApplicationBean.ApplicationType.PLUGIN;
+        } else if (ApplicationType.CROWD.equals(applicationType)) {
+            return ApplicationBean.ApplicationType.CROWD;
+        } else if (ApplicationType.JIRA.equals(applicationType)) {
+            return ApplicationBean.ApplicationType.JIRA;
+        } else if (ApplicationType.CONFLUENCE.equals(applicationType)) {
+            return ApplicationBean.ApplicationType.CONFLUENCE;
+        } else if (ApplicationType.STASH.equals(applicationType)) {
+            return ApplicationBean.ApplicationType.BITBUCKET;
+        } else if (ApplicationType.FISHEYE.equals(applicationType)) {
+            return ApplicationBean.ApplicationType.FISHEYE;
+        } else if (ApplicationType.CRUCIBLE.equals(applicationType)) {
+            return ApplicationBean.ApplicationType.CRUCIBLE;
+        } else if (ApplicationType.BAMBOO.equals(applicationType)) {
+            return ApplicationBean.ApplicationType.BAMBOO;
+        }


Da sich die gesamte Application Prodzedur eh nur in Crowd abspielt (und nicht in der commons, wo wir ja atlassian-free bleiben wollen), hätte man doch gleich Atlassian's ApplicationType anstelle von ApplicationBean.ApplicationType in die Bean aufnehmen können oder?

Prinzipiell schon, aber so behalten wir die Kontrolle und Bitbucket darf an unseren Endpoints Bitbucket heißen und nicht Stash.

src/main/java/de/aservo/confapi/crowd/rest/ApplicationsResourceImpl.java

…ionBean.

sonarqubecloud · 2020-10-07T07:44:26Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
0 Code Smells

93.6% Coverage
0.0% Duplication

pathob force-pushed the feature/applications-endpoint branch 3 times, most recently from 14a5960 to dace61d Compare September 30, 2020 08:48

pathob commented Oct 1, 2020

View reviewed changes

pathob force-pushed the feature/applications-endpoint branch 2 times, most recently from 1c80eeb to b8c03ff Compare October 1, 2020 07:49

pathob commented Oct 2, 2020

View reviewed changes

pathob commented Oct 5, 2020

View reviewed changes

cosminvlaicu force-pushed the feature/applications-endpoint branch from 0f0773d to 1dd7a5a Compare October 5, 2020 08:28

cosminvlaicu marked this pull request as ready for review October 5, 2020 08:48

pathob force-pushed the feature/applications-endpoint branch from 1dd7a5a to 0447ffa Compare October 5, 2020 13:23

cosminvlaicu force-pushed the feature/applications-endpoint branch from 0447ffa to e2c5b10 Compare October 5, 2020 13:30

pathob requested a review from lehmannk October 5, 2020 13:32

Implement applications endpoint

68592cb

pathob force-pushed the feature/applications-endpoint branch from e2c5b10 to dbecf77 Compare October 7, 2020 05:28

pathob marked this pull request as draft October 7, 2020 05:28

lehmannk reviewed Oct 7, 2020

View reviewed changes

Implemented tests for ApplicationsService Implementation and Applicat…

669680c

…ionBean.

cosminvlaicu force-pushed the feature/applications-endpoint branch from dbecf77 to 669680c Compare October 7, 2020 07:41

pathob mentioned this pull request Oct 8, 2020

Adjust API behavior regarding unset model attributes aservo/confapi-commons#92

Open

pathob marked this pull request as ready for review October 8, 2020 08:40

pathob requested a review from lehmannk October 8, 2020 08:40

lehmannk approved these changes Oct 8, 2020

View reviewed changes

pathob merged commit 4b41c63 into master Oct 12, 2020

pathob deleted the feature/applications-endpoint branch October 12, 2020 05:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement applications endpoint #16

Implement applications endpoint #16

pathob commented Sep 29, 2020

pathob left a comment

pathob left a comment

lehmannk Oct 6, 2020

pathob Oct 7, 2020

lehmannk Oct 7, 2020

pathob Oct 7, 2020

pathob Oct 8, 2020

lehmannk Oct 6, 2020

pathob Oct 7, 2020

sonarqubecloud bot commented Oct 7, 2020

Implement applications endpoint #16

Implement applications endpoint #16

Conversation

pathob commented Sep 29, 2020

pathob left a comment

Choose a reason for hiding this comment

pathob left a comment

Choose a reason for hiding this comment

lehmannk Oct 6, 2020

Choose a reason for hiding this comment

pathob Oct 7, 2020

Choose a reason for hiding this comment

lehmannk Oct 7, 2020

Choose a reason for hiding this comment

pathob Oct 7, 2020

Choose a reason for hiding this comment

pathob Oct 8, 2020

Choose a reason for hiding this comment

lehmannk Oct 6, 2020

Choose a reason for hiding this comment

pathob Oct 7, 2020

Choose a reason for hiding this comment

sonarqubecloud bot commented Oct 7, 2020