Skip to content

Commit

Permalink
fix: check dependencies when adding imports (smithy-lang#1239)
Browse files Browse the repository at this point in the history
* fix: check dependencies when adding imports

* avoid method overload with super type

* handle node: prefix package imports

* exempt node: prefix packages from enforced registration

* remove addImportUnchecked

* remove stray deprecation tag

* convert import logic to class

* comment grammar

* add internalapi annotation to ImportFrom
  • Loading branch information
kuhe authored Apr 12, 2024
1 parent ca9cb13 commit cad7ae0
Show file tree
Hide file tree
Showing 4 changed files with 226 additions and 5 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import java.util.function.UnaryOperator;
import software.amazon.smithy.codegen.core.CodegenException;
import software.amazon.smithy.codegen.core.Symbol;
import software.amazon.smithy.codegen.core.SymbolDependency;
import software.amazon.smithy.codegen.core.SymbolReference;
import software.amazon.smithy.codegen.core.SymbolWriter;
import software.amazon.smithy.model.Model;
Expand All @@ -29,6 +30,7 @@
import software.amazon.smithy.model.traits.DeprecatedTrait;
import software.amazon.smithy.model.traits.DocumentationTrait;
import software.amazon.smithy.model.traits.InternalTrait;
import software.amazon.smithy.typescript.codegen.validation.ImportFrom;
import software.amazon.smithy.utils.SmithyUnstableApi;
import software.amazon.smithy.utils.StringUtils;

Expand Down Expand Up @@ -114,19 +116,40 @@ public TypeScriptWriter addIgnoredDefaultImport(String name, String from, String
*/
@Deprecated
public TypeScriptWriter addImport(String name, String as, String from) {
ImportFrom importFrom = new ImportFrom(from);

if (importFrom.isDeclarablePackageImport()) {
String packageName = importFrom.getPackageName();
if (getDependencies()
.stream()
.map(SymbolDependency::getPackageName)
.noneMatch(packageName::equals)) {
throw new CodegenException(
"""
The import %s does not correspond to a registered dependency.
TypeScriptWriter::addDependency() is required before ::addImport().
""".formatted(from)
);
}
}

getImportContainer().addImport(name, as, from);
return this;
}

/**
* Imports a type using an alias from a module only if necessary.
* Adds the dependency.
*
* @param name Type to import.
* @param as Alias to refer to the type as.
* @param from PackageContainer to import the type from.
* @return Returns the writer.
*/
public TypeScriptWriter addImport(String name, String as, PackageContainer from) {
if (from instanceof Dependency) {
addDependency((Dependency) from);
}
return this.addImport(name, as, from.getPackageName());
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
/*
* Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
* SPDX-License-Identifier: Apache-2.0
*/

package software.amazon.smithy.typescript.codegen.validation;

import java.util.Set;
import software.amazon.smithy.utils.SetUtils;
import software.amazon.smithy.utils.SmithyInternalApi;

/**
* Interprets the string portion of an import statement.
*/
@SmithyInternalApi
public class ImportFrom {
public static final Set<String> NODE_NATIVE_DEPENDENCIES = SetUtils.of(
"buffer",
"child_process",
"crypto",
"dns",
"events",
"fs",
"http",
"http2",
"https",
"os",
"path",
"process",
"stream",
"tls",
"url",
"util",
"zlib"
);

private final String from;

public ImportFrom(String importTargetExpression) {
this.from = importTargetExpression;
}

/**
* @return whether we recognize it as a Node.js native module. These
* do not need to be declared in package.json. This check
* is not exhaustive since the list of native modules varies
* by version.
*/
public boolean isNodejsNative() {
String[] packageNameSegments = from.split("/");
return from.startsWith("node:")
|| NODE_NATIVE_DEPENDENCIES.contains(packageNameSegments[0]);
}

/**
* @return whether the import has an org or namespace prefix like \@smithy/pkg.
*/
public boolean isNamespaced() {
return from.startsWith("@") && from.contains("/");
}

/**
* @return whether the import starts with / or . indicating a relative import.
* These would not be added to package.json dependencies.
*/
public boolean isRelative() {
return from.startsWith("/") || from.startsWith(".");
}

/**
* @return whether the import should correspond to an entry in
* package.json.
*/
public boolean isDeclarablePackageImport() {
return !isNodejsNative() && !isRelative();
}

/**
* @return the package name. This excludes sub-paths of packages.
*
* For example in \@smithy/pkg/module the package name is \@smithy/pkg.
*/
public String getPackageName() {
String[] packageNameSegments = from.split("/");
String packageName;
if (isNodejsNative()) {
packageName = packageNameSegments[0].substring("node:".length());
} else if (isNamespaced()) {
packageName = packageNameSegments[0] + "/" + packageNameSegments[1];
} else {
packageName = packageNameSegments[0];
}
return packageName;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -21,15 +21,17 @@ public void writesDocStrings() {
public void doesNotAddNewlineBetweenManagedAndExplicitImports() {
TypeScriptWriter writer = new TypeScriptWriter("foo");
writer.write("import { Foo } from \"baz\";");
writer.addImport("Baz", "Baz", "hello");
writer.addImport("Baz", "Baz", "./hello");
writer.addImport("Bar", "__Bar", TypeScriptDependency.SMITHY_TYPES);
writer.addRelativeImport("Qux", "__Qux", Paths.get("./qux"));
String result = writer.toString();

assertThat(result, equalTo(CODEGEN_INDICATOR + "import { Qux as __Qux } from \"./qux\";\n"
+ "import { Bar as __Bar } from \"@smithy/types\";\n"
+ "import { Baz } from \"hello\";\n"
+ "import { Foo } from \"baz\";\n"));
assertThat(result, equalTo("""
%simport { Baz } from "./hello";
import { Qux as __Qux } from "./qux";
import { Bar as __Bar } from "@smithy/types";
import { Foo } from "baz";
""".formatted(CODEGEN_INDICATOR)));
}

@Test
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
package software.amazon.smithy.typescript.codegen.validation;

import org.junit.jupiter.api.Test;

import static org.junit.jupiter.api.Assertions.*;

class ImportFromTest {

@Test
void isNodejsNative() {
assertTrue(
new ImportFrom("node:buffer").isNodejsNative()
);
assertTrue(
new ImportFrom("stream").isNodejsNative()
);
assertFalse(
new ImportFrom("@smithy/util").isNodejsNative()
);
assertFalse(
new ImportFrom("../file").isNodejsNative()
);
}

@Test
void isNamespaced() {
assertTrue(
new ImportFrom("@smithy/util/submodule").isNamespaced()
);
assertTrue(
new ImportFrom("@smithy/util").isNamespaced()
);
assertFalse(
new ImportFrom("node:stream").isNamespaced()
);
assertFalse(
new ImportFrom("fs/promises").isNamespaced()
);
}

@Test
void isRelative() {
assertTrue(
new ImportFrom("/file/path").isRelative()
);
assertTrue(
new ImportFrom("./file/path").isRelative()
);
assertTrue(
new ImportFrom("../../../../file/path").isRelative()
);
assertFalse(
new ImportFrom("@smithy/util").isRelative()
);
assertFalse(
new ImportFrom("fs/promises").isRelative()
);
}

@Test
void isDeclarablePackageImport() {
assertTrue(
new ImportFrom("@smithy/util/submodule").isDeclarablePackageImport()
);
assertTrue(
new ImportFrom("@smithy/util").isDeclarablePackageImport()
);
assertTrue(
new ImportFrom("smithy_pkg").isDeclarablePackageImport()
);
assertTrue(
new ImportFrom("smithy_pkg/array").isDeclarablePackageImport()
);
assertFalse(
new ImportFrom("node:buffer").isDeclarablePackageImport()
);
assertFalse(
new ImportFrom("../pkg/pkg").isDeclarablePackageImport()
);
}

@Test
void getPackageName() {
assertEquals(
new ImportFrom("smithy_pkg/array").getPackageName(),
"smithy_pkg"
);
assertEquals(
new ImportFrom("@smithy/util/submodule").getPackageName(),
"@smithy/util"
);
assertEquals(
new ImportFrom("node:fs/promises").getPackageName(),
"fs"
);
assertEquals(
new ImportFrom("smithy_pkg").getPackageName(),
"smithy_pkg"
);
}
}

0 comments on commit cad7ae0

Please sign in to comment.