Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 20 additions & 28 deletions src/main/java/io/floci/cli/commands/RestartCommand.java
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,10 @@

import io.floci.cli.GlobalOptions;
import io.floci.cli.ProductProfile;
import io.floci.cli.config.Profile;
import io.floci.cli.config.ProfileStore;
import io.floci.cli.output.Printer;
import picocli.CommandLine.*;

import java.io.IOException;
import java.util.Optional;
import java.util.concurrent.Callable;

@Command(
Expand Down Expand Up @@ -44,6 +41,11 @@ public RestartCommand(ProductProfile profile, ProfileStore store) {
public Integer call() {
Printer printer = global.printer();

// Resolve the profile BEFORE stopping anything. A failure here must not leave the
// container stopped, and the values must not be re-read from a file that could change
// during the stop plus the one second wait below.
StartCommand start = buildStartCommand();

StopCommand stop = new StopCommand(profile);
stop.global = global;
stop.remove = false;
Expand All @@ -54,40 +56,30 @@ public Integer call() {
// Minimal wait to avoid port-already-in-use races
try { Thread.sleep(1000); } catch (InterruptedException e) { Thread.currentThread().interrupt(); }

return buildStartCommand().call();
return start.call();
}

/**
* The {@code start} this restart will run. Picocli never parses this instance, so the profile
* that {@code ProfileDefaultValueProvider} applied to the parsed command line has to be
* carried across by hand — without it, {@code restart --profile x} would silently drop the
* profile's persistence directory and service list.
* has to be applied by hand, and copying fields out of the Profile bean is not good enough:
* values reach a real {@code start} through picocli, which interpolates {@code ${env:HOME}}
* and friends. Going through {@link StartCommand#resolvedFor} keeps the two paths on the same
* provider, the same precedence and the same interpolation, so one profile cannot mean one
* directory on {@code start} and a different one on {@code restart}.
*/
public StartCommand buildStartCommand() {
StartCommand start = new StartCommand(profile);
StartCommand start = StartCommand.resolvedFor(profile, store, global.profile);

// The real invocation's globals win: they already carry the profile plus any flag the
// user passed, resolved once by the outer parse.
//
// Known residual: those globals come from the outer parse's read of the profile while the
// fields above come from resolvedFor's, so a profile edited between the two would mix
// snapshots. Closing it means making the outer provider reachable from commands, which is
// a wider change than this fix; tracked as batman LAY-1.
start.global = global;
start.pull = "missing";
start.detach = false;

// port and image already hold the product defaults, set by the StartCommand constructor.
if (global.profile != null) {
loadProfile().ifPresent(p -> {
if (p.port != null) start.port = p.port;
if (p.image != null) start.image = p.image;
if (p.persistDir != null) start.persistDir = p.persistDir;
if (p.services != null) start.services = p.services;
});
}
return start;
}

private Optional<Profile> loadProfile() {
try {
return store.get(global.profile);
} catch (IOException | IllegalArgumentException e) {
// Parsing already resolved this name, so a failure here cannot happen in practice;
// keeping the product defaults beats aborting a restart that has already stopped.
return Optional.empty();
}
}
}
41 changes: 41 additions & 0 deletions src/main/java/io/floci/cli/commands/StartCommand.java
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,13 @@

import io.floci.cli.GlobalOptions;
import io.floci.cli.ProductProfile;
import io.floci.cli.config.ProfileDefaultValueProvider;
import io.floci.cli.config.ProfileStore;
import io.floci.cli.docker.DockerClient;
import io.floci.cli.docker.DockerException;
import io.floci.cli.output.Ansi;
import io.floci.cli.output.Printer;
import picocli.CommandLine;
import picocli.CommandLine.*;

import java.net.URI;
Expand Down Expand Up @@ -54,6 +57,44 @@ protected StartCommand(ProductProfile profile) {
@Option(names = {"--pull"}, description = "Image pull policy: always, missing, never", defaultValue = "missing", paramLabel = "always|missing|never")
String pull;

/**
* A {@code StartCommand} with {@code profileName} applied by exactly the machinery a real
* {@code start} invocation uses: same provider, same precedence, same {@code ${...}}
* interpolation. Anything that needs to know what a profile would start with must go through
* here rather than reading the {@link io.floci.cli.config.Profile} bean, or it silently
* disagrees with {@code start} on any interpolated value.
*/
public static StartCommand resolvedFor(ProductProfile product, ProfileStore store, String profileName) {
return resolvedFor(product, new ProfileDefaultValueProvider(store), profileName);
}

/**
* As above, but with the provider supplied so a caller can read back the one profile snapshot
* it resolved ({@link ProfileDefaultValueProvider#resolved()}) instead of reading the file a
* second time and risking a mix of two versions.
*/
public static StartCommand resolvedFor(ProductProfile product,
ProfileDefaultValueProvider provider,
String profileName) {
StartCommand start = new StartCommand(product);
if (profileName != null) {
new CommandLine(start)
.setCaseInsensitiveEnumValuesAllowed(true)
.setDefaultValueProvider(provider)
.parseArgs("--profile", profileName);
}
return start;
}

/** What a profile-resolved instance would start with. Read by {@code config show}. */
public String image() { return image; }

public int port() { return port; }

public String persistDir() { return persistDir; }

public String services() { return services; }

/**
* The {@code docker run} arguments this invocation would use. Extracted from {@link #call()}
* as a test seam so the persistence mount can be pinned without starting a container;
Expand Down
39 changes: 24 additions & 15 deletions src/main/java/io/floci/cli/commands/config/ConfigShowCommand.java
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,15 @@

import io.floci.cli.GlobalOptions;
import io.floci.cli.ProductProfile;
import io.floci.cli.commands.StartCommand;
import io.floci.cli.config.Profile;
import io.floci.cli.config.ProfileDefaultValueProvider;
import io.floci.cli.config.ProfileStore;
import io.floci.cli.output.Ansi;
import io.floci.cli.output.OutputFormat;
import io.floci.cli.output.Printer;
import picocli.CommandLine.*;

import java.io.IOException;
import java.util.LinkedHashMap;
import java.util.Map;
import java.util.Optional;
Expand Down Expand Up @@ -58,12 +59,7 @@ public Integer call() {

// image/port/persistDir/services have no global option, so they are read back from the
// profile itself — they only take effect on 'start'.
startSettings().ifPresent(p -> {
if (p.image != null) data.put("image", p.image);
if (p.port != null) data.put("port", p.port);
if (p.persistDir != null) data.put("persistDir", p.persistDir);
if (p.services != null) data.put("services", p.services);
});
addStartSettings(data);

if (printer.format() != OutputFormat.text) {
printer.structured(data);
Expand All @@ -77,14 +73,27 @@ public Integer call() {
return 0;
}

private Optional<Profile> startSettings() {
if (global.profile == null) return Optional.empty();
try {
return store.get(global.profile);
} catch (IOException | IllegalArgumentException e) {
// Parsing already resolved this name; nothing useful to add here.
return Optional.empty();
}
// Values come from a profile-resolved StartCommand rather than the Profile bean, so an
// interpolated persistDir reads here exactly as 'start' would use it. The bean is consulted
// for presence, so a product default never shows up looking like a profile value.
//
// Both come from ONE snapshot: the provider memoizes the profile it read, so presence and
// values cannot disagree if another process edits the file mid-command. If the profile has
// gone since the outer parse resolved it, resolvedFor throws and the command fails loudly,
// which beats printing half of an old profile next to half of a new one.
private void addStartSettings(Map<String, Object> data) {
if (global.profile == null) return;

ProfileDefaultValueProvider provider = new ProfileDefaultValueProvider(store);
StartCommand resolved = StartCommand.resolvedFor(profile, provider, global.profile);
Optional<Profile> declared = provider.resolved();
if (declared.isEmpty()) return;

Profile p = declared.get();
if (p.image != null) data.put("image", resolved.image());
if (p.port != null) data.put("port", resolved.port());
if (p.persistDir != null) data.put("persistDir", resolved.persistDir());
if (p.services != null) data.put("services", resolved.services());
}

private static String label(String key) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,15 @@ public ProfileDefaultValueProvider(ProfileStore store) {
this.store = store;
}

/**
* The profile this provider resolved during the last parse, if any. Reading it costs nothing
* and, crucially, costs no second read of the file: a caller that needs both which keys the
* profile declares and what they expand to gets both from one snapshot.
*/
public Optional<Profile> resolved() {
return Optional.ofNullable(resolved);
}

@Override
public String defaultValue(ArgSpec arg) {
// Positional parameters reach the provider too; only options map to profile fields.
Expand Down
43 changes: 38 additions & 5 deletions src/main/java/io/floci/cli/config/ProfileStore.java
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.InvalidPathException;
import java.nio.file.Path;
import java.util.ArrayList;
import java.util.List;
Expand All @@ -17,7 +18,18 @@ public class ProfileStore {

// A profile name becomes a file name, so it must not be able to traverse out of the
// profiles directory: 'floci config profile delete ../../foo' resolved the raw name.
private static final Pattern VALID_NAME = Pattern.compile("[A-Za-z0-9._-]+");
//
// This is a deny-list on purpose. An allow-list over the whole character set also rejects
// names earlier versions accepted (a space, '+', '@'), which left those profiles listed by
// 'config profile list' and unreachable by show, --profile and delete, with no way to remove
// them through the CLI. The guarantee is carried by resolveInProfilesDir below, not by the
// character rules.
//
// Backslash is deliberately NOT here. Java NIO treats it as a separator on Windows, where
// resolveInProfilesDir already rejects '..\\..\\escape' on the parent check, and as an
// ordinary file-name character on Unix, where 0.2.1 could create 'team\\alpha.yaml' and
// list() still returns it. Denying it would orphan that profile on Unix for no gain.
private static final Pattern PATH_SEPARATOR = Pattern.compile("/");

private final Path profilesDir;

Expand All @@ -39,9 +51,9 @@ public static String validateName(String name) {
"Profile name must not be empty.\n"
+ "Run 'floci config profile list' to see available profiles.");
}
if (".".equals(name) || "..".equals(name) || !VALID_NAME.matcher(name).matches()) {
if (".".equals(name) || "..".equals(name) || PATH_SEPARATOR.matcher(name).find()) {
throw new IllegalArgumentException(
"Invalid profile name '" + name + "'. Use only letters, digits, '.', '_' and '-'.\n"
"Invalid profile name '" + name + "'. It must not be '.', '..', or contain '/'.\n"
+ "Run 'floci config profile list' to see available profiles.");
}
return name;
Expand Down Expand Up @@ -85,15 +97,36 @@ public boolean delete(String name) throws IOException {

/** Where {@link #save} writes {@code name}. Always {@code .yaml}. */
public Path profileFile(String name) {
return profilesDir.resolve(validateName(name) + ".yaml");
return resolveInProfilesDir(validateName(name) + ".yaml");
}

// The traversal guarantee: whatever the name looks like, the file it resolves to must sit
// directly in profilesDir. Also the place a name that is illegal on this platform but legal
// on another (a colon on Windows) turns into a clean message instead of InvalidPathException.
private Path resolveInProfilesDir(String fileName) {
Path dir = profilesDir.toAbsolutePath().normalize();
Path resolved;
try {
resolved = dir.resolve(fileName).normalize();
} catch (InvalidPathException e) {
throw new IllegalArgumentException(
"Invalid profile name '" + fileName + "': not a valid file name on this platform.\n"
+ "Run 'floci config profile list' to see available profiles.");
}
if (!dir.equals(resolved.getParent())) {
throw new IllegalArgumentException(
"Invalid profile name '" + fileName + "': it would resolve outside " + profilesDir + ".\n"
+ "Run 'floci config profile list' to see available profiles.");
}
return resolved;
}

// list() has always accepted .yml, so reads must too — otherwise a .yml profile shows up in
// 'config profile list' and then reports "not found" when passed to --profile.
private Path existingProfileFile(String name) {
Path yaml = profileFile(name);
if (Files.exists(yaml)) return yaml;
Path yml = profilesDir.resolve(validateName(name) + ".yml");
Path yml = resolveInProfilesDir(validateName(name) + ".yml");
return Files.exists(yml) ? yml : yaml;
}

Expand Down
10 changes: 10 additions & 0 deletions src/test/java/io/floci/cli/unit/ConfigCommandsTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,16 @@ void anExplicitFlagIsNoLongerDiscardedByTheProfile() throws Exception {
assertTrue(out.contains("\"container\" : \"floci-probe\""), out);
}

@Test
void reportsTheInterpolatedPersistDirThatStartWouldActuallyUse() throws Exception {
writeProfile("interp", "persistDir: ${env:HOME}/floci-data\n");

String out = runShow(ProductProfile.AWS, "--profile", "interp", "-o", "json");

assertTrue(out.contains("\"persistDir\" : \"" + System.getenv("HOME") + "/floci-data\""), out);
assertFalse(out.contains("${env:"), out);
}

@Test
void withoutAProfileItReportsTheProductDefaults() {
String out = runShow(ProductProfile.GCP, "-o", "json");
Expand Down
20 changes: 20 additions & 0 deletions src/test/java/io/floci/cli/unit/ProfilePrecedenceTest.java
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
package io.floci.cli.unit;

import io.floci.cli.FlociCli;
import io.floci.cli.ProductProfile;
import io.floci.cli.commands.StartCommand;
import io.floci.cli.config.ProfileDefaultValueProvider;
import io.floci.cli.config.ProfileStore;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.io.TempDir;
Expand Down Expand Up @@ -139,6 +142,23 @@ void commandsWithoutGlobalOptionsAreUntouched() {
leafSpec("update", "--check").findOption("--check").getValue());
}

/**
* config show reads presence and values from one snapshot by asking the provider what it
* resolved, rather than reading the file a second time. If this memo stops being populated,
* that command silently drops every start-only row.
*/
@Test
void theProviderRemembersTheProfileItResolved() throws Exception {
writeProfile("probe", FULL_PROFILE);
ProfileDefaultValueProvider provider = new ProfileDefaultValueProvider(new ProfileStore(tempDir));

StartCommand resolved = StartCommand.resolvedFor(ProductProfile.AWS, provider, "probe");

assertEquals("floci/floci:enforced", resolved.image());
assertEquals(4599, resolved.port());
assertEquals("floci-probe", provider.resolved().orElseThrow().container);
}

@Test
void picocliInterpolatesProfileValues() throws Exception {
// Documented consequence of routing values through picocli's default-value machinery.
Expand Down
Loading
Loading