fix: clean overrides for removed dependencies (#13170) (CP: 23.0)

flow-server/src/main/java/com/vaadin/flow/server/frontend/NodeUpdater.java

@@ -188,7 +188,8 @@ protected File getPackageLockFile() {
      * Gets the platform pinned versions that are not overridden by the user in
      * package.json.
      *
-     * @return json object with the dependencies or {@code null}
+     * @return {@code JsonObject} with the dependencies or empty
+     *         {@code JsonObject} if file doesn't exist
      * @throws IOException
      *             when versions file could not be read
      */
@@ -198,20 +199,16 @@ JsonObject getPlatformPinnedDependencies() throws IOException {
             log().info("Couldn't find {} file to pin dependency versions."
                     + " Transitive dependencies won't be pinned for pnpm.",
                     Constants.VAADIN_VERSIONS_JSON);
+            return Json.createObject();
         }
 
-        JsonObject versionsJson = null;
-        try (InputStream content = resource == null ? null
-                : resource.openStream()) {
-
-            if (content != null) {
-                VersionsJsonConverter convert = new VersionsJsonConverter(
-                        Json.parse(IOUtils.toString(content,
-                                StandardCharsets.UTF_8)));
-                versionsJson = convert.getConvertedJson();
-                versionsJson = new VersionsJsonFilter(getPackageJson(),
-                        DEPENDENCIES).getFilteredVersions(versionsJson);
-            }
+        JsonObject versionsJson;
+        try (InputStream content = resource.openStream()) {
+            VersionsJsonConverter convert = new VersionsJsonConverter(Json
+                    .parse(IOUtils.toString(content, StandardCharsets.UTF_8)));
+            versionsJson = convert.getConvertedJson();
+            versionsJson = new VersionsJsonFilter(getPackageJson(),
+                    DEPENDENCIES).getFilteredVersions(versionsJson);
         }
         return versionsJson;
     }

flow-server/src/main/java/com/vaadin/flow/server/frontend/TaskUpdatePackages.java

@@ -137,19 +137,29 @@ boolean lockVersionForNpm(JsonObject packageJson, String versionsPath)
         final JsonObject versionsJson = Json.parse(FileUtils.readFileToString(
                 generatedVersionsFile, StandardCharsets.UTF_8));
 
-        if (versionsJson != null) {
-            JsonObject overridesSection = getOverridesSection(packageJson);
-            final JsonObject dependencies = packageJson.getObject(DEPENDENCIES);
-            for (String dependency : versionsJson.keys()) {
-                if (!overridesSection.hasKey(dependency)
-                        && dependencies.hasKey(dependency)
-                        && !isInternalPseudoDependency(
-                                versionsJson.getString(dependency))) {
-                    overridesSection.put(dependency, "$" + dependency);
-                    versionLockingUpdated = true;
-                }
+        JsonObject overridesSection = getOverridesSection(packageJson);
+        final JsonObject dependencies = packageJson.getObject(DEPENDENCIES);
+        final JsonObject devDependencies = packageJson
+                .getObject(DEV_DEPENDENCIES);
+        for (String dependency : versionsJson.keys()) {
+            if (!overridesSection.hasKey(dependency)
+                    && (dependencies.hasKey(dependency)
+                            || devDependencies.hasKey(dependency))
+                    && !isInternalPseudoDependency(
+                            versionsJson.getString(dependency))) {
+                overridesSection.put(dependency, "$" + dependency);
+                versionLockingUpdated = true;
+            }
+        }
+        for (String dependency : overridesSection.keys()) {
+            if (!dependencies.hasKey(dependency)
+                    && !devDependencies.hasKey(dependency)
+                    && overridesSection.getString(dependency).startsWith("$")) {
+                overridesSection.remove(dependency);
+                versionLockingUpdated = true;
             }
         }
+
         return versionLockingUpdated;
     }
 
@@ -223,19 +233,17 @@ private boolean updatePackageJsonDependencies(JsonObject packageJson,
          */
         List<String> pinnedPlatformDependencies = new ArrayList<>();
         final JsonObject platformPinnedDependencies = getPlatformPinnedDependencies();
-        if (platformPinnedDependencies != null) {
-            for (String key : platformPinnedDependencies.keys()) {
-                // need to double check that not overriding a scanned
-                // dependency since add-ons should be able to downgrade
-                // version through exclusion
-                if (!applicationDependencies.containsKey(key)
-                        && pinPlatformDependency(packageJson,
-                                platformPinnedDependencies, key)) {
-                    added++;
-                }
-                // make sure platform pinned dependency is not cleared
-                pinnedPlatformDependencies.add(key);
+        for (String key : platformPinnedDependencies.keys()) {
+            // need to double check that not overriding a scanned
+            // dependency since add-ons should be able to downgrade
+            // version through exclusion
+            if (!applicationDependencies.containsKey(key)
+                    && pinPlatformDependency(packageJson,
+                            platformPinnedDependencies, key)) {
+                added++;
             }
+            // make sure platform pinned dependency is not cleared
+            pinnedPlatformDependencies.add(key);
         }
 
         if (added > 0) {

flow-server/src/test/java/com/vaadin/flow/server/frontend/TaskUpdatePackagesNpmTest.java

@@ -18,7 +18,6 @@
 
 import java.io.File;
 import java.io.IOException;
-import java.net.MalformedURLException;
 import java.nio.charset.StandardCharsets;
 import java.util.ArrayList;
 import java.util.Arrays;
@@ -247,6 +246,40 @@ public void npmIsInUse_versionJsonHasBadVersion_noFailureNothingAdded()
         verifyVersionLockingWithNpmOverrides(true, true, false);
     }
 
+    @Test
+    public void npmIsInUse_executionAfterDependencyRemoved_overlayIsCleanedOfDependency()
+            throws IOException {
+        createVaadinVersionsJson(PLATFORM_DIALOG_VERSION,
+                PLATFORM_ELEMENT_MIXIN_VERSION, PLATFORM_OVERLAY_VERSION);
+
+        final Map<String, String> applicationDependencies = createApplicationDependencies();
+        applicationDependencies.put(VAADIN_ELEMENT_MIXIN,
+                PLATFORM_ELEMENT_MIXIN_VERSION);
+        applicationDependencies.put(VAADIN_OVERLAY, PLATFORM_OVERLAY_VERSION);
+        TaskUpdatePackages task = createTask(applicationDependencies);
+        task.execute();
+        Assert.assertTrue("Updates not picked", task.modified);
+
+        verifyVersionLockingWithNpmOverrides(true, true, true);
+
+        // Remove platform lock for vaadin-element-mixin
+        final JsonObject versions = Json.parse(FileUtils
+                .readFileToString(versionJsonFile, StandardCharsets.UTF_8));
+        versions.getObject("core").remove("vaadin-element-mixin");
+        FileUtils.writeStringToFile(versionJsonFile, versions.toJson(),
+                StandardCharsets.UTF_8);
+
+        // Remove VAADIN_ELEMENT_MIXIN from the application dependencies
+        applicationDependencies.remove(VAADIN_ELEMENT_MIXIN);
+        task = createTask(applicationDependencies);
+
+        task.execute();
+
+        Assert.assertTrue("Updates not picked", task.modified);
+
+        verifyVersionLockingWithNpmOverrides(true, false, true);
+    }
+
     @Test
     public void npmIsInUse_versionsJsonHasSnapshotVersions_notAddedToPackageJson()
             throws IOException {
@@ -456,15 +489,18 @@ private void createVaadinVersionsJson(String dialogVersion,
             String elementMixinVersion, String overlayVersion) {
         // testing with exact versions json content instead of mocking parsing
         String versionJsonString = //@formatter:off
-                "{ \"core\": {" + "\"vaadin-dialog\": {\n"
+                "{ \"core\": {"
+                        + "\"vaadin-dialog\": {\n"
                         + "   \"component\": true,\n"
                         + "   \"javaVersion\": \"{{version}}\",\n"
                         + "    \"jsVersion\": \"" + dialogVersion + "\",\n"
                         + "    \"npmName\": \"" + VAADIN_DIALOG + "\"\n"
-                        + "},\n" + "\"vaadin-element-mixin\": {\n"
+                        + "},\n"
+                        + "\"vaadin-element-mixin\": {\n"
                         + "    \"jsVersion\": \"" + elementMixinVersion
                         + "\",\n" + "    \"npmName\": \"" + VAADIN_ELEMENT_MIXIN
-                        + "\"\n" + "},\n" + "\"vaadin-overlay\": {\n"
+                        + "\"\n" + "},\n"
+                        + "\"vaadin-overlay\": {\n"
                         + "    \"jsVersion\": \"" + overlayVersion + "\",\n"
                         + "    \"npmName\": \"" + VAADIN_OVERLAY + "\",\n"
                         + "    \"releasenotes\": true\n"
@@ -503,10 +539,10 @@ private TaskUpdatePackages createTask(
 
     private JsonObject getOrCreatePackageJson() throws IOException {
         File packageJson = new File(npmFolder, PACKAGE_JSON);
-        if (packageJson.exists())
+        if (packageJson.exists()) {
             return Json.parse(FileUtils.readFileToString(packageJson,
                     StandardCharsets.UTF_8));
-        else {
+        } else {
             final JsonObject packageJsonJson = Json.createObject();
             packageJsonJson.put(DEPENDENCIES, Json.createObject());
             FileUtils.writeStringToFile(new File(npmFolder, PACKAGE_JSON),
@@ -552,24 +588,28 @@ private void verifyVersionLockingWithNpmOverrides(boolean hasDialogLocking,
             Assert.assertEquals("$" + VAADIN_DIALOG,
                     overrides.getString(VAADIN_DIALOG));
         } else {
-            Assert.assertNull(overrides.get(VAADIN_DIALOG));
+            Assert.assertNull("vaadin-dialog dependency should not be present",
+                    overrides.get(VAADIN_DIALOG));
         }
         if (hasElementMixinLocking) {
             Assert.assertEquals("$" + VAADIN_ELEMENT_MIXIN,
                     overrides.getString(VAADIN_ELEMENT_MIXIN));
         } else {
-            Assert.assertNull(overrides.get(VAADIN_ELEMENT_MIXIN));
+            Assert.assertNull(
+                    "vaadin-element-mixin dependency should not be present",
+                    overrides.get(VAADIN_ELEMENT_MIXIN));
         }
         if (hasOverlayLocking) {
             Assert.assertEquals("$" + VAADIN_OVERLAY,
                     overrides.getString(VAADIN_OVERLAY));
         } else {
-            Assert.assertNull(overrides.get(VAADIN_OVERLAY));
+            Assert.assertNull("vaadin-overlay dependency should not be present",
+                    overrides.get(VAADIN_OVERLAY));
         }
     }
 
     private void verifyPlatformDependenciesAreAdded(boolean enablePnpm)
-            throws MalformedURLException, IOException {
+            throws IOException {
         Mockito.when(finder.getResource(Constants.VAADIN_VERSIONS_JSON))
                 .thenReturn(versionJsonFile.toURI().toURL());
         final String newVersion = "20.0.0";