refactor(web): Convert web module to Java #841

robzienert · 2020-05-28T01:15:16Z

When you're on a roll, you're on a roll.

This is the entire module.

…dable

robzienert · 2020-05-28T01:16:37Z

front50-core/src/main/groovy/com/netflix/spinnaker/front50/ServiceAccountsService.java

    return serviceAccountDAO.all();
  }

-  ServiceAccount createServiceAccount(ServiceAccount serviceAccount) {
+  public ServiceAccount createServiceAccount(ServiceAccount serviceAccount) {


These two methods were being accessed despite being out of scope.

robzienert · 2020-05-28T01:17:00Z

front50-web/front50-web.gradle

@@ -36,6 +36,7 @@ dependencies {
  implementation "org.springframework.boot:spring-boot-starter-actuator"
  implementation "com.netflix.spinnaker.fiat:fiat-api:$fiatVersion"
  implementation "com.netflix.spinnaker.fiat:fiat-core:$fiatVersion"
+  implementation "com.netflix.spinnaker.kork:kork-artifacts"


Groovy was referencing artifacts it didn't have access to.

robzienert · 2020-05-28T01:19:14Z

front50-web/src/main/java/com/netflix/spinnaker/front50/controllers/NotificationController.java

+              UntypedUtils.setProperty(notification, it, UntypedUtils.getProperty(global, it));
+            }
+          });
+    }


@ajordens Here's the global notifications. Maybe we can delete?

robzienert · 2020-05-28T01:20:32Z

front50-web/src/main/java/com/netflix/spinnaker/front50/controllers/v2/ProjectsController.java

+                (a, b) ->
+                    SearchUtils.score(b, normalizedAttributes)
+                        - SearchUtils.score(a, normalizedAttributes))
+            .collect(Collectors.toCollection(LinkedHashSet::new));


I broke this monstrosity up into a few blocks and methods. I'm happy to break it up further.

yikes... but yeah it looks like you captured the original essencé d'groovy - nice job

cfieber · 2020-05-28T16:50:51Z

front50-web/src/main/java/com/netflix/spinnaker/front50/controllers/NotificationController.java

+              if (!UntypedUtils.hasProperty(notification, it)) {
+                UntypedUtils.setProperty(notification, it, new ArrayList<>());
+              }
+              UntypedUtils.setProperty(notification, it, UntypedUtils.getProperty(global, it));


I think this needs to be:

Suggested change

UntypedUtils.setProperty(notification, it, UntypedUtils.getProperty(global, it));

((List<?>) UntypedUtils.getProperty(notification, it)).addAll((List<?>) UntypedUtils.getProperty(global, it)));

Otherwise it will replace the list instead of adding to it

Oops. Right you are.

ezimanyi

This is great! 💯 🏅 🏆 ☕

ezimanyi · 2020-05-28T14:42:50Z

...web/src/main/groovy/com/netflix/spinnaker/front50/controllers/v2/ApplicationsController.java

+
+    Set<Application> results =
+        new HashSet<>(
+            pageSize == null


This ternary looks inverted

ezimanyi · 2020-05-28T14:45:51Z

...0-web/src/main/groovy/com/netflix/spinnaker/front50/controllers/v2/EntityTagsController.java

+      @RequestParam(value = "prefix", required = false) final String prefix,
+      @RequestParam(value = "ids", required = false) Collection<String> ids,
+      @RequestParam(value = "refresh", required = false) Boolean refresh) {
+    if (prefix == null && (ids != null && !ids.isEmpty())) {


I think this might be incorrect; we'll throw an exception here if prefix == null but ids is a non-empty collection. Should this be prefix == null && ( ids == null || ids.isEmpty())?

Since we do the same check on ids later too, maybe we should just do an Optional.ofNullable(ids).orElseGet(ArrayList::new) at the beginning to simplify 🤷

ezimanyi · 2020-05-28T14:52:33Z

...t50-web/src/main/groovy/com/netflix/spinnaker/front50/controllers/v2/ProjectsController.java

+      @RequestParam(value = "pageSize", required = false) Integer pageSize,
+      @RequestParam Map<String, String> params) {
+    params.remove("pageSize");
+    Set<Project> projects =


Is this cast safe? Seems like we should either have the interface .all() return a Set or convert the result of projectDao.all() to a Set instead of just casting.

IntelliJ didn't complain about it so I figured it would be OK, but I've wrapped projectDAO.all() into new HashSet<>(projectDAO.all()) to be safe. Since all() is defined by ItemDAO<T>, I'm not comfortable in changing the signature of the method without affecting other places.

ezimanyi · 2020-05-28T14:54:24Z

...t50-web/src/main/groovy/com/netflix/spinnaker/front50/controllers/v2/ProjectsController.java

+          COMMA_SPLITTER.splitToList(normalizedAttributes.get("applications"));
+      // TODO(rz): LOL converting Groovy with elvis operators everywhere. :grimacing:
+      //  Do you know what's going on here? I certainly don't. Good thing there's tests?
+      items =


ezimanyi · 2020-05-28T16:56:37Z

...t50-web/src/main/groovy/com/netflix/spinnaker/front50/controllers/v2/ProjectsController.java

+                                    new FuzzySearch(project, it.getKey(), it.getValue()))))
+            .collect(Collectors.toSet()));
+
+    return new TreeSet<>(items)


Why do we need to make a TreeSet here instead of just doing items.stream()?

The signature was a Set, although I don't know why. The code was doing sorting on it, so I needed some implementation of SortedSet, which landed me on TreeSet - maybe this is the wrong implementation.

Alternatively I could change all of this to a List and just distinct on the stream before doing the sort? Kinda like this instead.

Oh I totally understand the result being a LinkedHashSet as we do seem to need it sorted...though I kind of like your idea better of making it a List with distinct.

I meant more that I think the following would be equivalent:

items.stream().sort()

new TreeSet<>(items).stream().sort()

so I wasn't sure it was wrapped right before calling .stream().

Oh boy, yeah ok, now I see what you're saying I missed the LinkedHashSet 🤦 .

This is now a List.

ezimanyi · 2020-05-28T17:01:19Z

front50-web/src/main/groovy/com/netflix/spinnaker/front50/controllers/PipelineController.java

+          if (p1.getIndex() == null && p2.getIndex() != null) {
+            return 1;
+          }
+          if (!p1.getIndex().equals(p2.getIndex())) {


I think this introduces a new NPE if both indexes are null.

Ha. Right you are. I guess this NPE has existed since... 2014. Will fix, good catch.

ezimanyi · 2020-05-28T17:03:50Z

front50-web/src/main/groovy/com/netflix/spinnaker/front50/controllers/PipelineController.java

+      @PathVariable(value = "application") String application,
+      @RequestParam(required = false, value = "refresh", defaultValue = "true") boolean refresh) {
+    List<Pipeline> pipelines =
+        (List<Pipeline>) pipelineDAO.getPipelinesByApplication(application, refresh);


Is this cast safe? If all implementations really do return this as a List should we change the type on the interface? If not should we create a list from the collection here?

I'm open to changing the interface to a list, but I think I'll try evaluating / taking that on as part of a separate PR.

cfieber

had one comment that I think is different behavior but otherwise LGTM, thanks again for doing this

can we update any of the gradle builds to disable the compileGroovy task for the modules that have been cut over at this point?

robzienert · 2020-05-28T18:41:15Z

@cfieber Taking a pass to remove compileGroovy is on the agenda once all this is merged!

clanesf · 2020-09-25T06:03:07Z

Thank you for the fix in Application create to actually check if fiat is enabled before attempting to make the role/sync call! Yet another reason to upgrade to 1.21!

robzienert added 18 commits May 27, 2020 15:08

refactor(web): Front50WebConfig to Java

f6f79c1

refactor(web): ItemDAOHealthIndicator to Java, move to better package

e92ee81

refactor(web): InvalidApplicationRequestException to Java

aa8d013

refactor(web): AuthorizationSupport to Java

910ea6a

refactor(web): PermissionsController to Java

19d03d6

refactor(web): ServiceAccountsController to Java

c38bd8a

refactor(web): Remove Groovy Slf4j annotation from AdminController

73f918f

refactor(web): NotificationController to Java

6323bd3

refactor(web): SnapshotsController and StrategyController to Java

d86090d

refactor(web): ApplicationsController to Java

82b9e89

refactor(web): EntityTagsController to Java

3d27dde

refactor(web): ProjectsController to Java

4e23620

refactor(web): Make ProjectsController a little less completely unrea…

360ef55

…dable

refactor(web): SimpleCORSFilter to Java

f123291

refactor(web): Main to Java

d210b58

refactor(web): PipelineController to Java

f93f576

refactor(web): Remove Groovy Slf4j annotation from DeliveryController

0f6385e

refactor(web): main source set to Java

7f2dbe9

robzienert requested review from cfieber, plumpy, ajordens, srekapalli, ezimanyi, dreynaud, jonsie and marchello2000 May 28, 2020 01:15

robzienert commented May 28, 2020

View reviewed changes

cfieber reviewed May 28, 2020

View reviewed changes

ezimanyi reviewed May 28, 2020

View reviewed changes

cfieber approved these changes May 28, 2020

View reviewed changes

robzienert added 5 commits May 28, 2020 11:25

fix(web): Add global notifications instead of replace

fc5e7a1

fix(web): Un-reverse ternary

da3e565

fix(web): Simplify entity tag ids conditional

35b5bb6

fix(web): Wrap all() in HashSet instead of perform maybe unsafe cast

7e45263

fix(web): Avoid NPE if both indexes are null

8b0be59

robzienert added 2 commits May 28, 2020 11:42

fix(web): Avoid potentially unsafe List cast

a2fae16

fix(web): Convert projects signature from Set to List, simplify logic

4a4fe18

robzienert force-pushed the death-to-web-groovy branch from 06f7863 to 4a4fe18 Compare May 28, 2020 20:02

ezimanyi approved these changes May 28, 2020

View reviewed changes

robzienert added the ready to merge Approved and ready for merge label May 28, 2020

mergify bot added the auto merged label May 28, 2020

Merge branch 'master' into death-to-web-groovy

cb2d3c5

mergify bot merged commit 7186db3 into spinnaker:master May 28, 2020

spinnakerbot added the target-release/1.21 label May 28, 2020

jervi mentioned this pull request Jun 8, 2020

fix(pipeline): Fix regression in Java conversion of PipelineController #859

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(web): Convert web module to Java #841

refactor(web): Convert web module to Java #841

robzienert commented May 28, 2020

robzienert May 28, 2020

robzienert May 28, 2020

robzienert May 28, 2020

robzienert May 28, 2020

cfieber May 28, 2020

cfieber May 28, 2020 •

edited

Loading

robzienert May 28, 2020

ezimanyi left a comment

ezimanyi May 28, 2020

ezimanyi May 28, 2020

ezimanyi May 28, 2020

ezimanyi May 28, 2020

robzienert May 28, 2020

ezimanyi May 28, 2020

ezimanyi May 28, 2020

robzienert May 28, 2020

ezimanyi May 28, 2020

robzienert May 28, 2020

ezimanyi May 28, 2020

robzienert May 28, 2020

ezimanyi May 28, 2020

robzienert May 28, 2020

cfieber left a comment

robzienert commented May 28, 2020

clanesf commented Sep 25, 2020

	UntypedUtils.setProperty(notification, it, UntypedUtils.getProperty(global, it));
	((List<?>) UntypedUtils.getProperty(notification, it)).addAll((List<?>) UntypedUtils.getProperty(global, it)));

refactor(web): Convert web module to Java #841

refactor(web): Convert web module to Java #841

Conversation

robzienert commented May 28, 2020

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

cfieber May 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ezimanyi 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

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

cfieber left a comment

Choose a reason for hiding this comment

robzienert commented May 28, 2020

clanesf commented Sep 25, 2020

cfieber May 28, 2020 •

edited

Loading