diff --git a/flow-devloop-daemon/README.md b/flow-devloop-daemon/README.md index dfcc571c90e..8658ebe3efd 100644 --- a/flow-devloop-daemon/README.md +++ b/flow-devloop-daemon/README.md @@ -422,6 +422,29 @@ its answer rather than claiming success. old one. This includes Spring Data repositories, which are bare interfaces with no annotation to spot them by, so the connector keys on the loaded proxy instead. +- **A bean or an entity the application has never seen must restart too.** + Component scanning runs once, at startup, over the classes that existed then, + and HA's Spring plugins that would rescan are disabled (below) — so a class + that is only now being given `@Component`, `@Service`, `@Repository`, + `@Controller`, `@RestController`, `@ControllerAdvice`, + `@RestControllerAdvice` or `@Configuration` gets no bean definition, and the + first injection point fails with `NoSuchBeanDefinitionException` naming + Spring rather than the loop. A brand-new `@Entity` is in exactly the same + position against a metamodel and a schema fixed at startup. It is the one + escalation with no redefine behind it: the class was never loaded, so there + is nothing to swap and every signal read off a loaded class is empty. It + takes both sides to say so, and deliberately: `REDEFINE` answers which of + the change-set's classes carry a stereotype (`stereotypes=`, read out of the + compiled bytes — the same reading `@Entity` already needed), and the + daemon's own inventory answers which of them the running application never + had. **Asking the app whether it has loaded the class does not work**: + HotswapAgent watches the output directory on its own schedule and defines a + new class when it sees one, so that answer flips between applies — and a + defined class is still not a bean definition. The inventory is re-seeded + from disk at every registration, so it does not flip. A stereotype composed + through a project's own meta-annotation is the known gap: only the custom + annotation is in the class's constant pool, so that one is still a restart + to ask for by hand. - **Hot-swap coverage differs sharply between stock HotSpot and a JBR.** Only a JBR gets `-XX:+AllowEnhancedClassRedefinition`; on stock HotSpot a structural change is simply rejected and escalates. A project needing a Java version no diff --git a/flow-devloop-daemon/src/main/java/com/vaadin/flow/devloop/daemon/Compile.java b/flow-devloop-daemon/src/main/java/com/vaadin/flow/devloop/daemon/Compile.java index 6ec927a524d..38a2e93b5c6 100644 --- a/flow-devloop-daemon/src/main/java/com/vaadin/flow/devloop/daemon/Compile.java +++ b/flow-devloop-daemon/src/main/java/com/vaadin/flow/devloop/daemon/Compile.java @@ -200,6 +200,17 @@ private static List joined(List first, List second) { private static final List PUBLIC_RESOURCE_ROOTS = List .of("META-INF/resources/", "static/", "public/", "resources/"); + /** + * What makes a file a Java source, and what a type's name is its file name + * minus. + */ + private static final String JAVA_SUFFIX = ".java"; + + /** + * What makes a file a class file, and what a binary name is its path minus. + */ + private static final String CLASS_SUFFIX = ".class"; + /** A file and the module it belongs to, which is all a walk ever needs. */ private interface Visitor { void accept(Reactor.Module module, Path file, Stamp stamp); @@ -208,6 +219,21 @@ private interface Visitor { /** Fingerprints of Java sources as of the last time they went live. */ private final Map applied = new java.util.concurrent.ConcurrentHashMap<>(); + /** + * The binary names of the class files the running application was launched + * with, as of the last seed. + *

+ * This is the whole of the answer to "has the application ever had this + * class?", and it has to be a snapshot rather than anything computed later. + * A timestamp cannot serve: the class file of a bean the application does + * run is rewritten by the first apply that hot swaps it, and every apply + * after that would then read it as a class the application never had - + * measured, the second method-body edit to a @Service restarted for a bean + * the context had held all along. + */ + private final Set launchedWith = java.util.concurrent.ConcurrentHashMap + .newKeySet(); + /** * Fingerprints as of the last browser notification, keyed by source path. */ @@ -724,7 +750,7 @@ List removeClassArtifacts(List sources) throws IOException { removed.add(artifact); } String nestedPrefix = fileName.toString().substring(0, - fileName.toString().length() - ".class".length()) + "$"; + fileName.toString().length() - CLASS_SUFFIX.length()) + "$"; for (Path nested : nestedClasses(directory, nestedPrefix)) { if (Files.deleteIfExists(nested)) { removed.add(nested); @@ -747,7 +773,7 @@ private static List nestedClasses(Path directory, String prefix) { return entries.filter(path -> { Path name = path.getFileName(); return name != null && name.toString().startsWith(prefix) - && name.toString().endsWith(".class"); + && name.toString().endsWith(CLASS_SUFFIX); }).toList(); } catch (IOException e) { return List.of(); @@ -787,7 +813,7 @@ List classpathForced(Launch.Project project) { continue; } walk(module, module.sourceDir(), - path -> path.toString().endsWith(".java"), + path -> path.toString().endsWith(JAVA_SUFFIX), (owner, file, stamp) -> forced.add(file)); } forced.sort(Comparator.naturalOrder()); @@ -805,6 +831,37 @@ List classpathChangedModules(Launch.Project project) { }).map(Reactor.Module::name).toList(); } + /** + * Which of these classes the running application never had. + *

+ * Asked per class rather than per source, which is the only way to get it + * right: a second top-level class or a nested one, added to a file the + * application has always had, is a class the application has never had. And + * answered from the snapshot taken when the application was launched, which + * is the only thing that stays true - an apply rewrites the class files it + * swaps, so anything read off the classpath afterwards says the + * application's own beans are strangers to it. + *

+ * A class compiled by an earlier apply is therefore still unknown, and + * rightly: the application has had it on the classpath since, but component + * scanning ran before it existed and no apply re-runs that. Only the + * restart does, and that re-seeds this. + * + * @param binaryNames + * the classes this apply compiled + * @return those of them the application never had, sorted + */ + List classesUnknownToTheApp(List binaryNames) { + return binaryNames.stream().filter(name -> !launchedWith.contains(name)) + .sorted(Comparator.naturalOrder()).toList(); + } + + private static String binaryNameOf(Reactor.Module module, Path classFile) { + String relative = module.classesDir().relativize(classFile).toString(); + return relative.substring(0, relative.length() - CLASS_SUFFIX.length()) + .replace(File.separatorChar, '.'); + } + /** Records that these sources are now live in the running JVM. */ void markSourcesApplied(List sources) { for (Path source : sources) { @@ -818,17 +875,33 @@ void markSourcesApplied(List sources) { * there. */ void seedFromDisk() { - seedFromDisk(Long.MAX_VALUE); + seedFromDisk(Long.MAX_VALUE, Long.MAX_VALUE); } /** + * @param startedAtMillis + * when the running application was launched, which bounds which + * sources it can have read * @param frontendCutoffMillis * how new a frontend file may be and still count as live; see * {@link #seedFrontend(long)} */ - void seedFromDisk(long frontendCutoffMillis) { + void seedFromDisk(long startedAtMillis, long frontendCutoffMillis) { + seedClasses(); applied.clear(); - forEachSource((module, source, stamp) -> applied.put(source, stamp)); + forEachSource((module, source, stamp) -> { + // Only what the application could have read. A source written + // after it was launched is not one it started with, whenever this + // baseline happens to be taken - and it can be taken late: the + // compile leg is built lazily, and a registration is handled a + // moment after the command that waited for it returned. Both are + // windows a developer's next keystroke fits into, and a baseline + // that walks "whatever is on disk now" claims those edits as the + // application's own. + if (stamp.modified() <= startedAtMillis) { + applied.put(source, stamp); + } + }); seedResources(); // Load-bearing for the frontend leg, not just tidiness: a bundled // frontend edit escalates to a restart, the restart re-registers, and @@ -838,10 +911,27 @@ void seedFromDisk(long frontendCutoffMillis) { seedFrontend(frontendCutoffMillis); } + /** + * The classpath as the application was launched with it, by binary name. + *

+ * One walk per application start, which is where a walk of the output + * directories belongs: every other answer about it is a comparison against + * this. + */ + private void seedClasses() { + launchedWith.clear(); + for (Reactor.Module module : modules) { + walk(module, module.classesDir(), + path -> path.toString().endsWith(CLASS_SUFFIX), + (owner, file, stamp) -> launchedWith + .add(binaryNameOf(owner, file))); + } + } + private void forEachSource(Visitor action) { for (Reactor.Module module : modules) { walk(module, module.sourceDir(), - path -> path.toString().endsWith(".java"), action); + path -> path.toString().endsWith(JAVA_SUFFIX), action); } } diff --git a/flow-devloop-daemon/src/main/java/com/vaadin/flow/devloop/daemon/Launch.java b/flow-devloop-daemon/src/main/java/com/vaadin/flow/devloop/daemon/Launch.java index 03b5c1ae856..e225cccf43e 100644 --- a/flow-devloop-daemon/src/main/java/com/vaadin/flow/devloop/daemon/Launch.java +++ b/flow-devloop-daemon/src/main/java/com/vaadin/flow/devloop/daemon/Launch.java @@ -317,6 +317,41 @@ Project project(Log progress) { } } + /** + * The resolved project if a sound one is already in hand, and never a + * resolution. + *

+ * For callers on a path that must not block: {@link #project()} runs Maven + * when the stamp has moved, which is seconds, and the registration + * connection is being answered on the thread that would wait for it. + *

+ * The fallback does not count. A resolution that wrote a current stamp and + * then failed to be read back leaves the application module alone standing + * in for the project, and a caller that builds on that builds on the wrong + * module set - which the next apply then reports as "module set changed" + * with no pom edit behind it. Such a caller is better off waiting for the + * apply that can resolve properly. + * + * @return the current project, or empty if resolving - or resolving again - + * is what it would take to have a sound one + */ + Optional projectIfResolved() { + Project current = project; + return current != null && !classpathUnusable && stampIsCurrent() + ? Optional.of(current) + : Optional.empty(); + } + + /** + * The daemon's own sink, for a caller that has nobody else to report to. + *

+ * Work triggered by an application registering has no client waiting on it, + * and what it has to say belongs in {@code daemon.log} rather than nowhere. + */ + Log log() { + return log; + } + /** * Set when {@link #project()} last had to fall back; empty when it is * sound. diff --git a/flow-devloop-daemon/src/main/java/com/vaadin/flow/devloop/daemon/TransactionEngine.java b/flow-devloop-daemon/src/main/java/com/vaadin/flow/devloop/daemon/TransactionEngine.java index df9567c28b4..f3969ac3ae0 100644 --- a/flow-devloop-daemon/src/main/java/com/vaadin/flow/devloop/daemon/TransactionEngine.java +++ b/flow-devloop-daemon/src/main/java/com/vaadin/flow/devloop/daemon/TransactionEngine.java @@ -283,7 +283,8 @@ private Compile compileFor(Launch.Project project, Launch.Log log) { // edit, first apply" sequence, and answering "no changes" to it is the // bug the frontend leg exists to fix. With no app running there is // nothing to be newer than: starting it re-seeds through onConnector. - fresh.seedFromDisk(app.startedAtMillis().orElse(Long.MAX_VALUE)); + long started = app.startedAtMillis().orElse(Long.MAX_VALUE); + fresh.seedFromDisk(started, started); // Said once per baseline rather than per apply: "why did apply not see // my edit?" is answerable from daemon.log only if the folder the daemon // decided on is written down somewhere. @@ -691,7 +692,14 @@ Transaction apply(Launch.Log log, boolean allowRestart) { Map fields = Connector.fields(reply.get()); tx.duplicates = parseInt(fields.get("dupes")); if ("OK".equals(fields.get("status"))) { - Optional blocker = blockedReason(fields); + // Per class rather than per source: a class added to a + // file the application has always had is still a class + // it has never had. Answered from the snapshot taken + // when the application was launched, so it does not + // matter that this apply has already written to the + // classpath by now. + Optional blocker = blockedReason(fields, + compile.classesUnknownToTheApp(tx.classes)); if (blocker.isEmpty()) { // What the JVM accepted, the app still has to run. blocker = loggedFailure(tx, log); @@ -1248,16 +1256,44 @@ private boolean notifyRemovedResources(List removed, Launch.Log log) { void onConnector(Connector connector) { this.connector = connector; + if (connector == null) { + return; + } Compile current = compile; - if (connector != null && current != null) { - // An app that has just registered is running exactly what is on - // disk, - // so that becomes the new "already live" baseline. Before the first - // apply there is no compile leg yet, and none is needed: the first - // one - // built seeds itself. - current.seedFromDisk(); + if (current == null) { + // Built here rather than left to the first apply, which is what + // this baseline used to wait for. "What the application started + // with" is only true of the disk at this moment: a compile leg + // built at the first apply instead seeds itself from a disk that + // has moved on, and a source added in between is then recorded as + // one the application has always had. Reported as a new + // @Component that hot-reloaded, and - when something else had + // already compiled it - as no change at all. + // + // Only from a classpath that is already resolved: this is the + // registration connection being answered, and it must not wait on + // Maven. A project that is mid-resolve leaves the baseline to the + // first apply, exactly as before. + // + // Seeded by compileFor as it is built, and deliberately not seeded + // again below: that seed carries the startup cutoff which keeps a + // frontend file edited while the application was starting visible + // to the first apply, and a second pass with no cutoff would + // declare it live and answer "no changes" over it. + if (launch != null) { + launch.projectIfResolved().ifPresent( + project -> compileFor(project, launch.log())); + } + return; } + // An app that has just registered is running exactly what is on disk - + // for the frontend, which a restart re-reads whole, so the cutoff + // there is open. Not for the Java side: this is handled a moment after + // the command that waited for the registration returned, and a source + // written in that moment is not something the application started + // with. + current.seedFromDisk(app.startedAtMillis().orElse(Long.MAX_VALUE), + Long.MAX_VALUE); } /** @@ -1518,11 +1554,19 @@ private String visibilityAdvice(Map fields) { } /** - * Cases where the JVM accepts the redefine but the change still is not - * live. Both were measured in P0.5, and both would otherwise be reported as + * Cases where the JVM accepts the redefine - or has nothing to redefine - + * and the change still is not live. Each would otherwise be reported as * {@code Stable} on an app that is stale or, worse, broken. + * + * @param fields + * the connector's reply, parsed + * @param unknownClasses + * the binary names of the change-set's classes the running + * application never had; see + * {@link Compile#classesUnknownToTheApp} */ - private Optional blockedReason(Map fields) { + static Optional blockedReason(Map fields, + List unknownClasses) { String entities = fields.getOrDefault("entities", "-"); if (!"-".equals(entities)) { return Optional.of("entity mapping cannot hot reload (" + entities @@ -1541,6 +1585,26 @@ private Optional blockedReason(Map fields) { + "): @JsModule and friends are read at startup" + " (dev bundle rebuild)"); } + // A bean the running application has never seen. Component scanning is + // a startup act, and HotswapAgent's Spring plugin - which would rescan + // - is disabled for stability (see Launch), so no mechanism short of a + // restart turns a new @Component into a bean definition. Without this + // the apply reports Stable and the view that injects the new bean + // fails with a NoSuchBeanDefinitionException that names Spring rather + // than the restart nobody was told to do. + // + // The app answers which of the change-set's classes carry a stereotype + // and the inventory answers which of them the app never had, because + // neither can answer both: a class the app has loaded may still be one + // it acquired seconds ago from HotswapAgent's watcher, and the daemon + // cannot read an annotation off a JVM it is not in. + String newBeans = unknownIn(fields.getOrDefault("stereotypes", "-"), + unknownClasses); + if (!newBeans.isEmpty()) { + return Optional.of("new Spring bean (" + newBeans + + "): component scanning ran at startup, so the running" + + " context has no definition for it"); + } // A method body inside a bean is fine: the proxy delegates to the // target // and the target's new body runs. What breaks is a change to the @@ -1575,6 +1639,35 @@ private Optional blockedReason(Map fields) { return Optional.empty(); } + /** + * The classes in one of the connector's {@code |}-separated lists that the + * running application never had, named as a reader would name them. + *

+ * Matched on binary names, which is why the connector reports that field + * under them: two classes in different packages can share a simple name, + * and one of them answering for the other is either a restart nobody needed + * or a bean nobody was told about. Only the message shortens them again. + * + * @param reported + * the field value, or {@code -} for none + * @param unknownClasses + * the binary names the application never had + * @return the matching classes by simple name, or empty for none + */ + private static String unknownIn(String reported, + List unknownClasses) { + if ("-".equals(reported) || unknownClasses.isEmpty()) { + return ""; + } + List matches = new ArrayList<>(); + for (String name : reported.split("\\|")) { + if (unknownClasses.contains(name)) { + matches.add(name.substring(name.lastIndexOf('.') + 1)); + } + } + return String.join("|", matches); + } + private static int parseInt(String value) { try { return value == null ? 0 : Integer.parseInt(value); diff --git a/flow-devloop-daemon/src/test/java/com/vaadin/flow/devloop/daemon/CompileTest.java b/flow-devloop-daemon/src/test/java/com/vaadin/flow/devloop/daemon/CompileTest.java index 22239bd090e..1793637984b 100644 --- a/flow-devloop-daemon/src/test/java/com/vaadin/flow/devloop/daemon/CompileTest.java +++ b/flow-devloop-daemon/src/test/java/com/vaadin/flow/devloop/daemon/CompileTest.java @@ -278,6 +278,61 @@ public class Main { } assertEquals(List.of(source(app, "Main")), changes.modified()); } + @Test + void classesUnknownToTheApp_answersFromTheLaunchSnapshot() + throws IOException { + // The signal a new bean or entity is escalated on. The application's + // own answer to "have you loaded this?" cannot serve: HotswapAgent's + // watcher defines a new class as soon as it notices the file, and a + // defined class is still not a bean definition. + Reactor.Module app = module("app", "Main", """ + package app; + public class Main { } + """); + Launch.Project project = project(app); + Compile compile = new Compile(project); + Path known = source(app, "Main"); + // What the application was launched with: one class on the classpath, + // and the baseline taken from it. + compile.compile(List.of(known), project); + compile.seedFromDisk(); + + // A class of its own in a file the application has always had, which + // is the case a per-source answer gets wrong. + assertEquals(List.of("app.Main$Inner", "app.Second"), + compile.classesUnknownToTheApp( + List.of("app.Main", "app.Main$Inner", "app.Second"))); + + // Compiled since - by an earlier apply, an IDE building on save, a + // bare mvn run. The application has it on the classpath now and still + // started without it, so the answer must not change. + Path added = known.resolveSibling("Added.java"); + Files.writeString(added, """ + package app; + public class Added { } + """); + compile.compile(List.of(added), project); + + assertEquals(List.of("app.Added"), compile + .classesUnknownToTheApp(List.of("app.Main", "app.Added"))); + + // And what the application does run stays its own however often it is + // recompiled, which is what keeps a second method-body edit a hot + // swap rather than a restart for a bean the context has always held. + compile.compile(List.of(known), project); + + assertTrue( + compile.classesUnknownToTheApp(List.of("app.Main")).isEmpty()); + + // Until the restart re-seeds, which is when the application has them + // all. + compile.seedFromDisk(); + + assertTrue( + compile.classesUnknownToTheApp(List.of("app.Main", "app.Added")) + .isEmpty()); + } + @Test void stale_reportsADeletedSourceAgainstTheInventory() throws IOException { // A walk only sees what is there, so the fingerprint map is what @@ -485,7 +540,7 @@ void staleFrontend_seedingKeepsAnEditMadeSinceTheAppStarted() long appStarted = System.currentTimeMillis(); touch("app/src/main/frontend/views/main.ts"); - compile.seedFromDisk(appStarted); + compile.seedFromDisk(appStarted, appStarted); Compile.FrontendChanges changes = compile.staleFrontend(); assertEquals( diff --git a/flow-devloop-daemon/src/test/java/com/vaadin/flow/devloop/daemon/LaunchTest.java b/flow-devloop-daemon/src/test/java/com/vaadin/flow/devloop/daemon/LaunchTest.java index 8cbd6a286a5..80e28e6c895 100644 --- a/flow-devloop-daemon/src/test/java/com/vaadin/flow/devloop/daemon/LaunchTest.java +++ b/flow-devloop-daemon/src/test/java/com/vaadin/flow/devloop/daemon/LaunchTest.java @@ -16,8 +16,12 @@ package com.vaadin.flow.devloop.daemon; import java.io.File; +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; @@ -86,4 +90,27 @@ void forwardedToApp_leavesTheDaemonsOwnJvmPropertiesBehind() { private static String classpath(String... entries) { return String.join(File.pathSeparator, entries); } + + @Test + void projectIfResolved_isEmptyUntilOneHasBeenResolved(@TempDir Path repo) + throws IOException { + // The baseline an application's registration builds is taken from this, + // on the thread answering that registration - so "nothing resolved + // yet" has to be an answer it can give rather than a Maven run it + // sets off. A caller that gets nothing here leaves the baseline to the + // first apply, which is where resolving belongs. + Files.createDirectories( + repo.resolve("src").resolve("main").resolve("java")); + Files.writeString(repo.resolve("pom.xml"), """ + + app + jar + + """); + Launch launch = new Launch(Reactor.discover(repo, text -> { + }), text -> { + }); + + assertTrue(launch.projectIfResolved().isEmpty()); + } } diff --git a/flow-devloop-daemon/src/test/java/com/vaadin/flow/devloop/daemon/TransactionEngineTest.java b/flow-devloop-daemon/src/test/java/com/vaadin/flow/devloop/daemon/TransactionEngineTest.java index cf18fcbdefc..36b0d5c19aa 100644 --- a/flow-devloop-daemon/src/test/java/com/vaadin/flow/devloop/daemon/TransactionEngineTest.java +++ b/flow-devloop-daemon/src/test/java/com/vaadin/flow/devloop/daemon/TransactionEngineTest.java @@ -93,6 +93,46 @@ void finish_keepsAFailureWorthReportingOnASupersededTransaction() { assertEquals("compile", tx.reason); } + @Test + void blockedReason_escalatesForABeanTheRunningApplicationHasNeverHad() { + // Two half-answers make this verdict: the app says which classes carry + // a stereotype, the change-set says which of them the application + // never had. Read from the reply alone the apply answers Stable, and + // the view that injects the new bean fails with Spring's own exception + // instead. + String reply = "OK redefined=1 beans=- structural=-" + + " stereotypes=com.example.Extra|com.example.TaskService"; + + assertEquals( + Optional.of("new Spring bean (Extra): component scanning ran" + + " at startup, so the running context has no" + + " definition for it"), + TransactionEngine.blockedReason(Connector.fields(reply), + List.of("com.example.Extra"))); + // The bean the app started with is named in the same field and must + // not escalate: a method-body change inside it is exactly what the + // runtime leg exists to swap. + assertTrue(TransactionEngine + .blockedReason(Connector.fields(reply), List.of()).isEmpty()); + // Matched on the binary name, so a class that merely shares a simple + // name with a new one answers for nothing: reporting it would be a + // restart nobody needed. + assertTrue(TransactionEngine.blockedReason( + Connector.fields("OK stereotypes=com.example.Extra" + + " entities=- structural=-"), + List.of("com.example.other.Extra")).isEmpty()); + // And a nested type is its own class, reported under its own name. + assertEquals( + Optional.of("new Spring bean (Outer$Inner): component scanning" + + " ran at startup, so the running context has no" + + " definition for it"), + TransactionEngine.blockedReason( + Connector + .fields("OK stereotypes=com.example.Outer$Inner" + + " entities=- structural=-"), + List.of("com.example.Outer$Inner"))); + } + @Test void devServerFailure_isTheVerdictForAnErrorLoggedWhenTheFileWasSaved() { // Vite compiles on save, so its error is in the log before apply even diff --git a/flow-tests/test-devloop/devloop-app/src/test/java/com/vaadin/flow/devloop/test/it/DevLoopApplyIT.java b/flow-tests/test-devloop/devloop-app/src/test/java/com/vaadin/flow/devloop/test/it/DevLoopApplyIT.java index 306187838da..ec786304898 100644 --- a/flow-tests/test-devloop/devloop-app/src/test/java/com/vaadin/flow/devloop/test/it/DevLoopApplyIT.java +++ b/flow-tests/test-devloop/devloop-app/src/test/java/com/vaadin/flow/devloop/test/it/DevLoopApplyIT.java @@ -46,6 +46,30 @@ void methodBodyEdit_isHotSwappedRatherThanRestarted() { outcome.assertOutputDoesNotContain("restarting"); } + @Test + void aBeanEditedTwiceOverStaysAHotSwapBothTimes() { + // A bean, and two edits in a row, which is the shape that catches a + // "has the application ever had this class?" answer built out of + // timestamps: the first apply rewrites the class file, and a second + // apply that reads only "this class file is newer than the launch" + // then mistakes the application's own bean for one it has never + // scanned - and restarts for it. + java.nio.file.Path bean = MUTABLE.resolve("TaskService.java"); + + patch.replace(bean, "\"Write the plan\"", "\"Write the plan, once\""); + cli.run("apply").assertExitCode(0).assertOutputContains("hot-reload:") + .assertOutputDoesNotContain("restarting"); + + patch.replace(bean, "\"Write the plan, once\"", + "\"Write the plan, twice\""); + + VaadinDevCli.Outcome outcome = cli.run("apply").assertExitCode(0); + + outcome.assertOutputContains("hot-reload:"); + outcome.assertOutputDoesNotContain("restarting"); + outcome.assertOutputDoesNotContain("new Spring bean"); + } + @Test void compileError_failsWithADiagnosticAndKeepsTheAppRunning() { patch.replace(MUTABLE.resolve("TaskListView.java"), diff --git a/flow-tests/test-devloop/devloop-app/src/test/java/com/vaadin/flow/devloop/test/it/DevLoopRestartIT.java b/flow-tests/test-devloop/devloop-app/src/test/java/com/vaadin/flow/devloop/test/it/DevLoopRestartIT.java index 2987d38dd8e..808169da3c4 100644 --- a/flow-tests/test-devloop/devloop-app/src/test/java/com/vaadin/flow/devloop/test/it/DevLoopRestartIT.java +++ b/flow-tests/test-devloop/devloop-app/src/test/java/com/vaadin/flow/devloop/test/it/DevLoopRestartIT.java @@ -15,16 +15,28 @@ */ package com.vaadin.flow.devloop.test.it; +import javax.tools.ToolProvider; + +import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; import java.nio.file.Path; +import java.util.stream.Stream; +import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; + +import static org.junit.jupiter.api.Assertions.assertEquals; /** * The changes a redefine cannot make live, and the reason each one gives. *

- * Both cases here are ones that used to be reported as live: the JVM accepts - * the redefine, and what the application built from the old class at startup - - * a bean's proxy, an ORM's metamodel - silently no longer matches. + * Every case here is one that used to be reported as live: what the application + * built at startup - a bean's proxy, an ORM's metamodel, the set of bean + * definitions - silently no longer matches the sources, and the redefine that + * says so is either accepted or not needed at all. */ class DevLoopRestartIT extends AbstractDevLoopIT { @@ -66,6 +78,198 @@ void addingAnEntityAnnotation_escalatesEvenThoughTheClassRedefines() { "entity mapping cannot hot reload (TaskListView)"); } + @ParameterizedTest(name = "@{0}") + @ValueSource(strings = { "Component", "Service" }) + void aNewSpringBean_escalatesEvenThoughNothingWasRedefined( + String stereotype) { + // The case with no redefine behind it: a class the running JVM has + // never loaded has nothing to swap, so every signal the two cases above + // turn on is empty and the apply reported Stable. Component scanning is + // a startup act, though, so the context has no definition for the new + // bean and the first view to inject it fails with Spring's own + // NoSuchBeanDefinitionException - which names Spring rather than the + // restart nobody was told to do. + // + // The change-set is deliberately one new file and no edit to a loaded + // class. Taking the new bean as a constructor parameter of a view that + // is already running would restart on a stock JVM whatever this + // reports, because adding a parameter is a structural change and + // redefineClasses rejects it - so the fixture would pass without the + // rule it is here to pin, and only a JVM with enhanced class + // redefinition would show the difference. + // + // Two stereotypes rather than one, and only two: @Component is the + // one every other is composed from and @Service the one an + // application reaches for, so between them they show the rule is + // about the change-set rather than about a particular annotation. + // That each remaining entry of the list matches its own descriptor is + // pinned in DevLoopRedefinerTest, where it costs no application. + // + // What this one cannot see, and did not: setUp applies before the + // fixture exists, so the daemon's baseline is already built and + // already seeded by the time the file appears. A miss that needs the + // file to appear *before* the first apply - which is what a developer + // does, and what was reported - is invisible from here however many + // stereotypes it runs. That ordering has its own case below. + String type = "Extra" + stereotype; + patch.create(MUTABLE.resolve(type + ".java"), """ + package com.vaadin.flow.devloop.test.app.mutable; + + import org.springframework.stereotype.%1$s; + + /** Created by DevLoopRestartIT and deleted again by it. */ + @%1$s + public class %2$s { + } + """.formatted(stereotype, type)); + + // --no-restart stops at the verdict, as in the case above. + VaadinDevCli.Outcome outcome = cli + .run("apply", "--no-restart", "--json").assertExitCode(0); + + outcome.assertOutputContains("new Spring bean (" + type + ")"); + } + + @Test + void aSecondClassInAFileTheAppAlreadyHas_stillEscalates() { + // A class of its own, in a source the application has always had. Read + // per source rather than per class, the file is one the application + // read at startup and the answer comes back "known" - while the class + // beside it is one the application has never seen and has no bean + // definition for. + // + // A second top-level class rather than a nested one: nesting rewrites + // the enclosing class's NestMembers attribute, which redefineClasses + // rejects outright, so that fixture would escalate on the JVM's word + // and prove nothing about this rule. Appending leaves the enclosing + // class byte-identical. + patch.append(MUTABLE.resolve("TaskService.java"), """ + + @org.springframework.stereotype.Component + class ExtraInTheSameFile { + } + """); + + VaadinDevCli.Outcome outcome = cli + .run("apply", "--no-restart", "--json").assertExitCode(0); + + outcome.assertOutputContains("new Spring bean (ExtraInTheSameFile)"); + } + + @Test + void aClassAnnotatedAfterAnEarlierApply_stillEscalates() { + // The class was compiled by an apply, not by the build the application + // started from, so component scanning has never seen it however many + // applies have. An answer that treats "this apply put it on the + // classpath" as "the application has it" reports the second apply as a + // hot swap over a bean that does not exist. + Path source = MUTABLE.resolve("ExtraLater.java"); + patch.create(source, """ + package com.vaadin.flow.devloop.test.app.mutable; + + /** Created by DevLoopRestartIT and deleted again by it. */ + public class ExtraLater { + } + """); + cli.run("apply").assertExitCode(0); + + patch.replace(source, "public class ExtraLater {", + "@org.springframework.stereotype.Component\npublic class ExtraLater {"); + + VaadinDevCli.Outcome outcome = cli + .run("apply", "--no-restart", "--json").assertExitCode(0); + + outcome.assertOutputContains("new Spring bean (ExtraLater)"); + } + + @ParameterizedTest(name = "compiled by the daemon: {0}") + @ValueSource(booleans = { true, false }) + void aNewSpringBean_escalatesOnTheFirstApplyOfADaemonsLife( + boolean daemonCompilesIt) throws IOException { + // Same rule as above, in the ordering that got past it. The baseline + // the rule is read against describes what the application started + // with, and it used to be taken when the compile leg was first needed + // - the first apply - by which time a source created since the start + // is on disk and lands in it as though the application had always had + // it. Every other test in this class applies once in setUp, which is + // exactly what hides this: by then the baseline is already taken. + // + // The file has to be created after the start: a start builds the + // module, so a source already on disk would be compiled and scanned + // into the application it is meant to be missing from. + cli.run("shutdown").assertExitCode(0); + cli.run("start").assertExitCode(0); + Path source = MUTABLE.resolve("ExtraBean.java"); + patch.create(source, """ + package com.vaadin.flow.devloop.test.app.mutable; + + import org.springframework.stereotype.Component; + + /** Created by DevLoopRestartIT and deleted again by it. */ + @Component + public class ExtraBean { + } + """); + if (!daemonCompilesIt) { + // An IDE building on save, or a plain mvn run, in the same window. + // The artifact is then newer than the source and the baseline has + // the stamp, so the change-set did not see the file at all and the + // apply answered "no changes" - a worse answer than the wrong + // verdict, and the same root cause. + compileOutsideTheDaemon(source); + } + + VaadinDevCli.Outcome outcome = cli + .run("apply", "--no-restart", "--json").assertExitCode(0); + + outcome.assertOutputContains("new Spring bean (ExtraBean)"); + outcome.assertOutputDoesNotContain("no changes"); + } + + /** + * Compiles a fixture the way anything other than the daemon would, into the + * classpath the application is running. + */ + private static void compileOutsideTheDaemon(Path source) + throws IOException { + // The application's own classpath, as the daemon resolved it when it + // launched: this JVM's may be a manifest-only jar, which would not + // resolve the annotation. + String classpath = Files.readString( + APP.resolve("target").resolve("devloop").resolve("cp.txt"), + StandardCharsets.UTF_8).trim(); + int status = ToolProvider.getSystemJavaCompiler().run(null, null, null, + "-nowarn", "-proc:none", "-classpath", classpath, "-d", + APP.resolve("target").resolve("classes").toString(), + source.toString()); + assertEquals(0, status, + "the fixture has to compile for this test to mean anything"); + } + + /** + * The fixtures here are new classes, and a class file whose source was + * created and deleted without a restart in between is deliberately left for + * the next build - so these take their own artifacts out. Otherwise the + * next application to start would component-scan a bean this class + * invented, and the test after it would be measuring that. + */ + @AfterEach + void removeFixtureArtifacts() throws IOException { + Path classes = APP.resolve("target").resolve("classes") + .resolve("com/vaadin/flow/devloop/test/app/mutable".replace('/', + java.io.File.separatorChar)); + if (!Files.isDirectory(classes)) { + return; + } + try (Stream artifacts = Files.list(classes)) { + for (Path artifact : artifacts.filter( + path -> path.getFileName().toString().startsWith("Extra")) + .toList()) { + Files.deleteIfExists(artifact); + } + } + } + @Test void restart_bringsTheAppBackOnTheSamePort() { cli.run("restart").assertExitCode(0); diff --git a/vaadin-dev-server/src/main/java/com/vaadin/base/devserver/devloop/DevLoopRedefiner.java b/vaadin-dev-server/src/main/java/com/vaadin/base/devserver/devloop/DevLoopRedefiner.java index ff3e7dfbd0b..a664313a852 100644 --- a/vaadin-dev-server/src/main/java/com/vaadin/base/devserver/devloop/DevLoopRedefiner.java +++ b/vaadin-dev-server/src/main/java/com/vaadin/base/devserver/devloop/DevLoopRedefiner.java @@ -105,6 +105,12 @@ final class DevLoopRedefiner { */ private static final String CLASSES_PROPERTY = "vaadin.devloop.classes"; + /** + * The free-text tail of a reply. Always last, and the only field that may + * contain a space, so the daemon reads it as the rest of the line. + */ + private static final String MESSAGE = " message="; + /** * The directory names that make a public resource root, as whole path * segments so a match cannot land in the middle of one. @@ -115,6 +121,33 @@ final class DevLoopRedefiner { "/META-INF/resources/", "/static/", "/public/", "/resources/", "/webapp/" }; + /** + * The annotations {@link #isEntity} asks a loaded class for, as they are + * spelled in a class file. + */ + private static final List ENTITY_DESCRIPTORS = List.of( + "Ljakarta/persistence/Entity;", + "Ljakarta/persistence/MappedSuperclass;", + "Ljakarta/persistence/Embeddable;"); + + /** + * The stereotypes that register a bean, as they are spelled in a class + * file. {@code @RestController} and the two advice annotations are listed + * in their own right because each is a {@code @Component} through a + * meta-annotation that the annotated class's own constant pool does not + * mention - see {@link #declaresSpringBean} for the ones that cannot be + * listed. + */ + private static final List BEAN_DESCRIPTORS = List.of( + "Lorg/springframework/stereotype/Component;", + "Lorg/springframework/stereotype/Service;", + "Lorg/springframework/stereotype/Repository;", + "Lorg/springframework/stereotype/Controller;", + "Lorg/springframework/web/bind/annotation/RestController;", + "Lorg/springframework/web/bind/annotation/ControllerAdvice;", + "Lorg/springframework/web/bind/annotation/RestControllerAdvice;", + "Lorg/springframework/context/annotation/Configuration;"); + /** * Where Vite puts the failure in the error page it serves for a module it * could not transform - the JSON its own overlay renders. @@ -179,7 +212,8 @@ private DevLoopRedefiner() { static String redefine(String csv) { Instrumentation inst = instrumentation(); if (inst == null) { - return "ERR kind=no-agent message=Instrumentation-unavailable"; + return "ERR kind=no-agent" + MESSAGE + + "Instrumentation-unavailable"; } Hotswapper hotswapper = DevLoopRegistration.hotswapper().orElse(null); if (hotswapper == null) { @@ -189,7 +223,7 @@ static String redefine(String csv) { List requested = Arrays.stream(csv.split(",")).map(String::trim) .filter(name -> !name.isEmpty()).toList(); if (requested.isEmpty()) { - return "ERR kind=protocol message=no-classes"; + return "ERR kind=protocol" + MESSAGE + "no-classes"; } List classesDirs = searchPath(); @@ -207,47 +241,12 @@ static String redefine(String csv) { } } - List definitions = new ArrayList<>(); - List notLoaded = new ArrayList<>(); - int duplicates = 0; - Set entities = new LinkedHashSet<>(); - Set beans = new LinkedHashSet<>(); - Set uiClasses = new LinkedHashSet<>(); - - for (String name : requested) { - List> targets = loaded.getOrDefault(name, List.of()); - if (targets.isEmpty()) { - notLoaded.add(name); - continue; - } - if (targets.size() > 1) { - duplicates += targets.size() - 1; - } - Class first = targets.get(0); - // The class as the application has been running it, which answers - // for an annotation the change is taking away: a type that stops - // being an entity was still mapped by the metamodel the application - // started with. - classify(first, entities, beans, uiClasses); - byte[] bytes = readClassBytes(classesDirs, name); - if (bytes == null) { - return "ERR kind=missing-class-file searched=" - + classesDirs.size() + " message=" + name; - } - // And the class the JVM is about to be given. A type that is only - // now being made an entity is not one yet in the loop above, and - // Hibernate mapped neither version: the metamodel and the schema - // were fixed at startup. Asked of the bytes rather than of the - // class after the redefine, because the loaded class is not a - // reliable witness to what it has just been given - see - // declaresEntity. - if (declaresEntity(bytes)) { - entities.add(simple(name)); - } - for (Class target : targets) { - definitions.add(new ClassDefinition(target, bytes)); - } + Inspection inspected = inspect(requested, loaded, classesDirs); + if (inspected.error() != null) { + return inspected.error(); } + // The one part of it this method works with rather than only reports. + List definitions = inspected.definitions(); DevLoopHotswapper observer = DevLoopHotswapper.getActive(); if (observer != null) { @@ -275,7 +274,7 @@ static String redefine(String csv) { definitions.toArray(new ClassDefinition[0])); } catch (Throwable t) { return "ERR kind=redefine-rejected class=" - + t.getClass().getSimpleName() + " message=" + + t.getClass().getSimpleName() + MESSAGE + oneLine(String.valueOf(t.getMessage())); } } @@ -311,14 +310,177 @@ static String redefine(String csv) { boolean pageReload = observer != null && observer.isPageReloadRequired(); - return "OK redefined=" + definitions.size() + " notLoaded=" - + notLoaded.size() + " dupes=" + duplicates + " completed=" - + completed + " pageReload=" + pageReload + " entities=" - + join(entities) + " beans=" + join(beans) + " proxied=" - + join(proxied) + " structural=" + join(structural) + " ui=" - + join(uiClasses) + " frontendImports=" + join(frontend) - + " hotswapAgent=" + hotswapAgentLoaded() + " redefineMs=" - + redefineMs + " hotswapMs=" + hotswapMs; + return reply(inspected, new Applied(structural, proxied, frontend, + completed, pageReload, redefineMs, hotswapMs)); + } + + /** + * What the redefine, and the refresh that followed it, turned out to do. + * + * @param structural + * redefined types whose shape changed + * @param proxied + * redefined types a live proxy was generated from + * @param frontend + * redefined types whose build-time imports changed + * @param completed + * whether Flow's refresh ran to the end + * @param pageReload + * whether Flow asked for the page to be reloaded + * @param redefineMs + * how long {@code redefineClasses} took + * @param hotswapMs + * how long {@code onHotswap} took + */ + record Applied(Set structural, Set proxied, + Set frontend, boolean completed, boolean pageReload, + long redefineMs, long hotswapMs) { + } + + /** + * The one line the daemon reads a verdict out of. + *

+ * Every field here is part of the wire contract: the daemon splits the line + * on whitespace and reads by name, so a renamed or dropped field is a + * silently different answer rather than a parse failure. A field it does + * not know is ignored, which is what makes adding one safe in either + * direction. + * + * @param inspected + * what the request amounted to + * @param applied + * what happened to it + * @return the reply line + */ + static String reply(Inspection inspected, Applied applied) { + return "OK redefined=" + inspected.definitions().size() + " notLoaded=" + + inspected.notLoaded().size() + " dupes=" + + inspected.duplicates() + " completed=" + applied.completed() + + " pageReload=" + applied.pageReload() + " entities=" + + join(inspected.entities()) + " beans=" + + join(inspected.beans()) + " proxied=" + + join(applied.proxied()) + " structural=" + + join(applied.structural()) + " ui=" + + join(inspected.uiClasses()) + " frontendImports=" + + join(applied.frontend()) + " hotswapAgent=" + + hotswapAgentLoaded() + " redefineMs=" + applied.redefineMs() + + " hotswapMs=" + applied.hotswapMs() + // Last rather than in among the others, so adding it left every + // field a daemon already reads exactly where it was. + + " stereotypes=" + join(inspected.stereotypes()); + } + + /** + * What the requested names amount to before anything is redefined. + * + * @param definitions + * every loaded copy paired with the bytes to give it + * @param notLoaded + * requested names this JVM has no class for + * @param duplicates + * how many extra loaded copies were found + * @param entities + * types mapped as JPA entities before or after the change + * @param beans + * types the application is running as Spring beans + * @param stereotypes + * types whose new bytes carry a Spring stereotype + * @param uiClasses + * types {@code onHotswap} visibly refreshes + * @param error + * the reply to send instead of redefining, or {@code null} + */ + record Inspection(List definitions, List notLoaded, + int duplicates, Set entities, Set beans, + Set stereotypes, Set uiClasses, String error) { + } + + /** + * Reads what one {@code REDEFINE} request is asking for: which loaded + * copies to hand the JVM, and what the classes and their new bytes say + * about whether a redefine can be the whole answer. + *

+ * Separated from {@link #redefine} because this is the whole of the + * decision and none of the effect - it defines nothing, refreshes nothing + * and needs no running application - which is also what makes it the part + * that can be tested without one. + * + * @param requested + * the requested binary names + * @param loaded + * every loaded copy of them, by binary name + * @param classesDirs + * where to read the new bytes from, in classpath order + * @return what the request amounts to + */ + static Inspection inspect(List requested, + Map>> loaded, List classesDirs) { + List definitions = new ArrayList<>(); + List notLoaded = new ArrayList<>(); + int duplicates = 0; + Set entities = new LinkedHashSet<>(); + Set beans = new LinkedHashSet<>(); + Set stereotypes = new LinkedHashSet<>(); + Set uiClasses = new LinkedHashSet<>(); + + for (String name : requested) { + List> targets = loaded.getOrDefault(name, List.of()); + if (targets.isEmpty()) { + notLoaded.add(name); + } else { + if (targets.size() > 1) { + duplicates += targets.size() - 1; + } + // The class as the application has been running it, which + // answers for an annotation the change is taking away: a type + // that stops being an entity was still mapped by the metamodel + // the application started with. + classify(targets.get(0), entities, beans, uiClasses); + } + byte[] bytes = readClassBytes(classesDirs, name); + if (bytes == null) { + if (targets.isEmpty()) { + // Neither loaded here nor on this search path, so there is + // nothing to redefine and nothing to answer for. + continue; + } + return new Inspection(definitions, notLoaded, duplicates, + entities, beans, stereotypes, uiClasses, + "ERR kind=missing-class-file searched=" + + classesDirs.size() + MESSAGE + name); + } + // And the class the JVM is about to be given. A type that is only + // now being made an entity is not one yet in the classify above, + // and Hibernate mapped neither version: the metamodel and the + // schema were fixed at startup. Asked of the bytes rather than of + // the class after the redefine, because the loaded class is not a + // reliable witness to what it has just been given - see + // declaresEntity. + if (declaresEntity(bytes)) { + entities.add(simple(name)); + } + // Reported for every requested class rather than only for the ones + // this JVM has not loaded, and deliberately: whether the + // application ever *had* this class is the daemon's question, not + // this one's. HotswapAgent watches the output directory on its own + // schedule and defines a new class when it sees one, so "not + // loaded here" is a race, and losing it would report a brand-new + // bean as live. What the bytes say is not a race, and the daemon + // knows which of these classes it has just brought into being. + // + // Under its binary name, unlike every other field here: this one + // is read by machine and matched against the change-set, and two + // classes in different packages can share a simple name - which + // would make one of them answer for the other. + if (declaresSpringBean(bytes)) { + stereotypes.add(name); + } + for (Class target : targets) { + definitions.add(new ClassDefinition(target, bytes)); + } + } + return new Inspection(definitions, notLoaded, duplicates, entities, + beans, stereotypes, uiClasses, null); } /** @@ -1007,12 +1169,6 @@ private static boolean isEntity(Class type) { "jakarta.persistence.Embeddable"); } - /** The same annotations, as they are spelled in a class file. */ - private static final List ENTITY_DESCRIPTORS = List.of( - "Ljakarta/persistence/Entity;", - "Ljakarta/persistence/MappedSuperclass;", - "Ljakarta/persistence/Embeddable;"); - /** * Whether the compiled bytes carry a JPA annotation, read from the class * file rather than from the class once it is loaded. @@ -1036,12 +1192,46 @@ private static boolean isEntity(Class type) { * needed, and the cost of a false negative is {@code Stable} over a mapping * the application never had. */ - private static boolean declaresEntity(byte[] bytes) { + static boolean declaresEntity(byte[] bytes) { + return declares(bytes, ENTITY_DESCRIPTORS); + } + + /** + * Whether the compiled bytes carry a Spring stereotype, read from the class + * file because there is no loaded class to ask. + *

+ * This is the question {@link #isSpringBean} cannot answer: it reports what + * the application has been running with, and the class this is asked about + * is one the application has never run at all. + *

+ * It errs in both directions, unlike {@link #declaresEntity}, and the + * asymmetry is worth knowing before trusting the answer. A descriptor in + * the constant pool is not proof that the annotation is on the class - it + * could sit on a member, or be a type the class merely mentions - which + * costs a restart that was not needed. In the other direction, a stereotype + * composed through a meta-annotation is invisible: a project's own + * {@code @MyService}, itself annotated {@code @Service}, puts only + * {@code @MyService} in this class's pool, and the annotation type whose + * pool would say the rest is a separate class file. + * {@link #BEAN_DESCRIPTORS} names the composed stereotypes Spring itself + * ships; a project's own are a restart its author still has to ask for, and + * the same limit as {@link #hasAnnotation}, which resolves one level of + * meta-annotation and no more. + */ + static boolean declaresSpringBean(byte[] bytes) { + return declares(bytes, BEAN_DESCRIPTORS); + } + + /** + * Whether any of these annotation descriptors appears in the class file's + * constant pool. + */ + private static boolean declares(byte[] bytes, List descriptors) { // ISO-8859-1 maps every byte to the char of the same value, so a // substring search over it is an exact byte search - and a descriptor // is ASCII, which the class file's modified UTF-8 encodes unchanged. String constants = new String(bytes, StandardCharsets.ISO_8859_1); - return ENTITY_DESCRIPTORS.stream().anyMatch(constants::contains); + return descriptors.stream().anyMatch(constants::contains); } /** diff --git a/vaadin-dev-server/src/test/java/com/vaadin/base/devserver/devloop/DevLoopRedefinerTest.java b/vaadin-dev-server/src/test/java/com/vaadin/base/devserver/devloop/DevLoopRedefinerTest.java index bbdb2199619..6be8aead3a7 100644 --- a/vaadin-dev-server/src/test/java/com/vaadin/base/devserver/devloop/DevLoopRedefinerTest.java +++ b/vaadin-dev-server/src/test/java/com/vaadin/base/devserver/devloop/DevLoopRedefinerTest.java @@ -16,11 +16,20 @@ package com.vaadin.base.devserver.devloop; import java.io.File; +import java.io.IOException; +import java.io.InputStream; +import java.net.URISyntaxException; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.Paths; +import java.util.Arrays; import java.util.List; +import java.util.Map; +import java.util.Set; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; import com.vaadin.flow.component.Component; import com.vaadin.flow.component.Tag; @@ -30,6 +39,7 @@ import com.vaadin.flow.theme.Theme; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -37,9 +47,12 @@ * The connector must always answer exactly one line, even on failure, or the * daemon blocks waiting for a reply that is not coming. *

- * Only the no-agent and no-hotswapper paths are unit-testable: everything past - * them needs a real {@code Instrumentation} handle and a running application. - * The rest is covered end to end in {@code flow-tests/test-devloop}. + * Of the reply itself, what is unit-testable is what needs no running + * application: the no-agent and no-hotswapper paths, the decision + * {@code inspect} reaches about a change-set, and the questions asked of a + * class or of its compiled bytes. Defining a class and refreshing Flow need a + * real {@code Instrumentation} handle and a live service, and are covered end + * to end in {@code flow-tests/test-devloop}. */ class DevLoopRedefinerTest { @@ -183,6 +196,191 @@ void frontendDependencies_readsAComponentAndAnswersEmptyForNeither() { DevLoopRedefiner.frontendDependencies(NothingDeclared.class)); } + @Test + void declaresFromBytes_answersForAClassNothingHasLoaded() + throws IOException { + // The only questions that can be asked about a class the JVM has never + // loaded, and both have to be, because a context that scanned before + // the class existed has no bean definition for it and a metamodel + // built then maps no entity that appeared afterwards. + byte[] plain = classBytes(NothingDeclared.class); + + assertFalse(DevLoopRedefiner.declaresSpringBean(plain), "plain class"); + assertFalse(DevLoopRedefiner.declaresEntity(plain), "plain class"); + // Every entry, spelled out rather than read from the production list - + // which would pass whatever that list happened to say. A stereotype is + // matched by exactly one literal, so a typo in one of them is one + // annotation that silently stops escalating while the rest keep + // working, and this is where that is cheap to rule out. Spelled out + // rather than compiled in for a second reason too: Spring is not on + // this module's classpath, and what the check reads is the descriptor + // javac writes into the constant pool. + // + // @RestController and the two advice annotations are listed in their + // own right because each is a @Component through a meta-annotation + // that the annotated class's own constant pool never mentions. + for (String stereotype : List.of( + "Lorg/springframework/stereotype/Component;", + "Lorg/springframework/stereotype/Service;", + "Lorg/springframework/stereotype/Repository;", + "Lorg/springframework/stereotype/Controller;", + "Lorg/springframework/web/bind/annotation/RestController;", + "Lorg/springframework/web/bind/annotation/ControllerAdvice;", + "Lorg/springframework/web/bind/annotation/RestControllerAdvice;", + "Lorg/springframework/context/annotation/Configuration;")) { + assertTrue(DevLoopRedefiner.declaresSpringBean( + withConstant(plain, stereotype)), stereotype); + } + // The two escalate on separate fields and carry separate reasons, so + // neither may answer for the other. + assertFalse(DevLoopRedefiner.declaresSpringBean( + withConstant(plain, "Ljakarta/persistence/Entity;"))); + assertTrue(DevLoopRedefiner.declaresEntity( + withConstant(plain, "Ljakarta/persistence/Entity;"))); + } + + private static byte[] classBytes(Class type) throws IOException { + try (InputStream in = DevLoopRedefinerTest.class.getResourceAsStream( + "/" + type.getName().replace('.', '/') + ".class")) { + return in.readAllBytes(); + } + } + + /** + * A real class file with one more entry in its constant pool, which is what + * the check reads and all it reads. + */ + private static byte[] withConstant(byte[] bytes, String descriptor) { + byte[] added = descriptor.getBytes(StandardCharsets.ISO_8859_1); + byte[] joined = Arrays.copyOf(bytes, bytes.length + added.length); + System.arraycopy(added, 0, joined, bytes.length, added.length); + return joined; + } + + @Test + void inspect_readsALoadedClassAndTheBytesItIsAboutToBeGiven() { + String plain = NothingDeclared.class.getName(); + String view = SomeView.class.getName(); + + // The same class twice is what a duplicate loaded copy looks like, and + // both have to go into the one redefine call: redefining one leaves + // the copy the application instantiates untouched, which is a green + // apply over a stale page. + DevLoopRedefiner.Inspection inspected = DevLoopRedefiner.inspect( + List.of(plain, view), + Map.of(plain, + List.of(NothingDeclared.class, NothingDeclared.class), + view, List.of(SomeView.class)), + List.of(testClasses())); + + assertNull(inspected.error()); + assertEquals(3, inspected.definitions().size()); + assertEquals(1, inspected.duplicates()); + assertTrue(inspected.notLoaded().isEmpty()); + // What the loaded class says, which is the other source and reaches + // the daemon on its own field: onHotswap visibly refreshes a + // Component, and a change-set with none is reported as live but not + // yet visible rather than simply stable. Under the binary tail of the + // name, which is what a nested type is reported as. + assertEquals(Set.of("DevLoopRedefinerTest$SomeView"), + inspected.uiClasses()); + // Neither of them is a bean or an entity, by either source: the + // redefine is the whole of this change. + assertTrue(inspected.entities().isEmpty(), "entities"); + assertTrue(inspected.stereotypes().isEmpty(), "stereotypes"); + assertTrue(inspected.beans().isEmpty(), "beans"); + } + + @Test + void inspect_namesTheBeansAndEntitiesThisJvmHasNoClassFor( + @TempDir Path classes) throws IOException { + // A class that did not exist when the application started. There is + // nothing loaded to redefine, so every signal read off a loaded class + // is empty and only the new bytes say anything - and what they say is + // that component scanning and the metamodel, both of which ran at + // startup, know nothing about these. + writeClass(classes, "bean.NewBean", + "Lorg/springframework/stereotype/Service;"); + writeClass(classes, "model.NewEntity", "Ljakarta/persistence/Entity;"); + + DevLoopRedefiner.Inspection inspected = DevLoopRedefiner.inspect( + List.of("bean.NewBean", "model.NewEntity", "gone.Nothing"), + Map.of(), List.of(classes)); + + // The third one is neither loaded nor on the search path - a class + // from outside the loop - and is no error and nothing to answer for. + assertNull(inspected.error()); + assertTrue(inspected.definitions().isEmpty()); + assertEquals(List.of("bean.NewBean", "model.NewEntity", "gone.Nothing"), + inspected.notLoaded()); + // Binary names, because the daemon matches this field against the + // change-set and a simple name is not an identity. + assertEquals(Set.of("bean.NewBean"), inspected.stereotypes()); + assertEquals(Set.of("NewEntity"), inspected.entities()); + } + + @Test + void inspect_aLoadedClassWithNoNewBytesIsTheOneError() { + // The daemon compiled it before asking, so bytes that are not on the + // search path mean the path is wrong. Redefining the rest of the + // change-set would report success over a class that never got them. + String name = NothingDeclared.class.getName(); + + DevLoopRedefiner.Inspection inspected = DevLoopRedefiner.inspect( + List.of(name), Map.of(name, List.of(NothingDeclared.class)), + List.of(Path.of("no", "such", "directory"))); + + assertEquals("ERR kind=missing-class-file searched=1 message=" + name, + inspected.error()); + } + + @Test + void reply_carriesEveryFieldTheDaemonReadsAVerdictFrom() { + // The daemon splits this line on whitespace and reads by name, so a + // renamed or dropped field is a silently different answer rather than + // a parse failure - which is why the whole line is asserted. + DevLoopRedefiner.Inspection inspected = new DevLoopRedefiner.Inspection( + List.of(), List.of("gone.Nothing"), 1, Set.of("Order"), + Set.of("TaskService"), Set.of("com.example.NewBean"), + Set.of("TaskListView"), null); + DevLoopRedefiner.Applied applied = new DevLoopRedefiner.Applied( + Set.of("TaskService"), Set.of("TaskRepository"), + Set.of("TaskListView"), true, false, 4, 7); + + // hotswapAgent is read off this JVM, which has no agent on its + // classpath. + assertEquals( + "OK redefined=0 notLoaded=1 dupes=1 completed=true" + + " pageReload=false entities=Order beans=TaskService" + + " proxied=TaskRepository structural=TaskService" + + " ui=TaskListView frontendImports=TaskListView" + + " hotswapAgent=false redefineMs=4 hotswapMs=7" + + " stereotypes=com.example.NewBean", + DevLoopRedefiner.reply(inspected, applied)); + } + + /** The module's own test output, which is a real classpath directory. */ + private static Path testClasses() { + try { + return Path.of(DevLoopRedefinerTest.class.getProtectionDomain() + .getCodeSource().getLocation().toURI()); + } catch (URISyntaxException e) { + throw new AssertionError(e); + } + } + + /** + * A class file for a name nothing has loaded, carrying one annotation + * descriptor in its constant pool - which is all these checks read. + */ + private static void writeClass(Path root, String binaryName, + String descriptor) throws IOException { + Path file = root.resolve(binaryName.replace('.', '/') + ".class"); + Files.createDirectories(file.getParent()); + Files.write(file, + withConstant(classBytes(NothingDeclared.class), descriptor)); + } + @Test void frontend_carriesTheFieldsTheDaemonDecidesModeWith() { String reply = DevLoopRedefiner.frontend(null);