Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: clean overrides for removed dependencies #13170

Merged
merged 2 commits into from
Feb 28, 2022
Merged

fix: clean overrides for removed dependencies #13170

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
19ed1c2
fix: clean overrides for removed dependencies
caalador Feb 28, 2022
ebc775d
if simplification, javadoc fix
caalador Feb 28, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
23 changes: 10 additions & 13 deletions flow-server/src/main/java/com/vaadin/flow/server/frontend/NodeUpdater.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 JsonObject with the dependencies or empty JsonObject if file
* doesn't exist
joheriks marked this conversation as resolved.
Show resolved Hide resolved
* @throws IOException
* when versions file could not be read
*/
Expand All @@ -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;
}
Expand Down
54 changes: 31 additions & 23 deletions flow-server/src/main/java/com/vaadin/flow/server/frontend/TaskUpdatePackages.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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("$")) {
joheriks marked this conversation as resolved.
Show resolved Hide resolved
overridesSection.remove(dependency);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it mean that any $-overrides will be removed, even some added by the user?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Note: certainly not blocking, as this may not be a real issue as this mechanism is unlikely to be used much in Flow projects, just asking for the record)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This checks the dependencies and devDependencies sections, right? So if I have a dependency on foo I can define an override for foo without this removing it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So it will remove overrides starting with $ that are not in dependencies or devDependencies. As such a configuration would cause npm to raise an error anyway this is probably unlikely to be an issue ever.

versionLockingUpdated = true;
}
}

return versionLockingUpdated;
}

Expand Down Expand Up @@ -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) {
Expand Down
60 changes: 50 additions & 10 deletions flow-server/src/test/java/com/vaadin/flow/server/frontend/TaskUpdatePackagesNpmTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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"
Expand Down Expand Up @@ -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),
Expand Down Expand Up @@ -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";
Expand Down