Copilot commented on code in PR #769: URL: https://github.com/apache/fesod/pull/769#discussion_r2652345122
########## fesod-cli/src/main/java/org/apache/fesod/cli/formatters/FormatterFactory.java: ########## @@ -0,0 +1,50 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.fesod.cli.formatters; + +import java.util.HashMap; +import java.util.Map; +import org.apache.fesod.cli.exception.CliException; + +/** + * Formatter factory + */ +public class FormatterFactory { + + private static final Map<String, OutputFormatter> FORMATTERS = new HashMap<String, OutputFormatter>(); + + static { + registerFormatter(new JsonFormatter()); + registerFormatter(new CsvFormatter()); + registerFormatter(new XmlFormatter()); + } + + public static void registerFormatter(OutputFormatter formatter) { + FORMATTERS.put(formatter.getFormatType().toLowerCase(), formatter); + } + + public static OutputFormatter getFormatter(String formatType) { + OutputFormatter formatter = FORMATTERS.get(formatType.toLowerCase()); + if (formatter == null) { + throw new CliException("Unsupported format: " + formatType); Review Comment: There is an extra space in the error message. It should be "Unsupported format:" without the double space before the format type. ```suggestion throw new CliException("Unsupported format: " + formatType); ``` ########## fesod-cli/src/main/java/org/apache/fesod/cli/FesodCli.java: ########## @@ -0,0 +1,96 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.fesod.cli; + +import org.apache.fesod.cli.commands.ConvertCommand; +import org.apache.fesod.cli.commands.InfoCommand; +import org.apache.fesod.cli.commands.ReadCommand; +import org.apache.fesod.cli.commands.VersionCommand; +import org.apache.fesod.cli.commands.WriteCommand; +import org.apache.fesod.cli.exception.CliException; +import picocli.CommandLine; +import picocli.CommandLine.Command; +import picocli.CommandLine.Model.CommandSpec; +import picocli.CommandLine.Option; +import picocli.CommandLine.Spec; + +@Command( + name = "fesod-cli", + mixinStandardHelpOptions = true, + version = {"Apache Fesod CLI", "Java Runtime: ${java.version}", "OS: ${os.name} ${os.arch}"}, + description = "Fast and Easy spreadsheet processing from the command line", + subcommands = { + ReadCommand.class, + WriteCommand.class, + ConvertCommand.class, + InfoCommand.class, + VersionCommand.class, + CommandLine.HelpCommand.class + }, + usageHelpAutoWidth = true, + footer = { + "", + "Examples:", + " fesod-cli read data.xlsx --format json", + " fesod-cli convert input.xls output.xlsx", + " fesod-cli info data.xlsx", + "", + "Documentation: https://fesod.apache.org/docs/cli";, + "Report bugs: https://github.com/apache/fesod/issues"; Review Comment: There is an extra space in the footer. It should be "Report bugs: https://github.com/apache/fesod/issues"; without the double space before the URL. ```suggestion "Documentation: https://fesod.apache.org/docs/cli";, "Report bugs: https://github.com/apache/fesod/issues"; ``` ########## fesod-cli/src/main/java/org/apache/fesod/cli/core/sheet/SheetReader.java: ########## @@ -0,0 +1,99 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.fesod.cli.core.sheet; + +import com.alibaba.fastjson2.JSONArray; +import com.alibaba.fastjson2.JSONObject; +import java.nio.file.Path; +import java.util.ArrayList; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; +import org.apache.fesod.sheet.ExcelReader; +import org.apache.fesod.sheet.FesodSheet; +import org.apache.fesod.sheet.read.metadata.ReadSheet; + +/** + * Sheet reader implementation + */ +public class SheetReader { + + public Map<String, Object> read(Path inputPath, Integer sheetIndex, String sheetName, Boolean readAll) { + Map<String, Object> result = new LinkedHashMap<String, Object>(); + + if (readAll) { + result.put("sheets", readAllSheets(inputPath)); + } else if (sheetName != null) { + result.put("data", readSheetByName(inputPath, sheetName)); + } else { + int index = sheetIndex != null ? sheetIndex : 0; + result.put("data", readSheetByIndex(inputPath, index)); + } + + return result; + } + + private List<Map<String, Object>> readAllSheets(Path inputPath) { + List<Map<String, Object>> allSheets = new ArrayList<Map<String, Object>>(); + List<ReadSheet> sheets; + + try (ExcelReader excelReader = FesodSheet.read(inputPath.toFile()).build()) { + sheets = excelReader.excelExecutor().sheetList(); + } + + for (ReadSheet sheet : sheets) { + Map<String, Object> sheetData = new LinkedHashMap<String, Object>(); + sheetData.put("name", sheet.getSheetName()); + sheetData.put("index", sheet.getSheetNo()); + sheetData.put("rows", readSheetByIndex(inputPath, sheet.getSheetNo())); + allSheets.add(sheetData); + } + Review Comment: The ExcelReader is opened within a try-with-resources block but then immediately closed before the sheets are read. The code attempts to iterate over the sheets list after the reader is closed, which could lead to issues. The sheet data reading (line 65) happens outside the try-with-resources block, which means it's operating on potentially stale sheet metadata. Consider restructuring to read all sheet data while the ExcelReader is still open. ```suggestion try (ExcelReader excelReader = FesodSheet.read(inputPath.toFile()).build()) { List<ReadSheet> sheets = excelReader.excelExecutor().sheetList(); for (ReadSheet sheet : sheets) { Map<String, Object> sheetData = new LinkedHashMap<String, Object>(); sheetData.put("name", sheet.getSheetName()); sheetData.put("index", sheet.getSheetNo()); sheetData.put("rows", readSheetByIndex(inputPath, sheet.getSheetNo())); allSheets.add(sheetData); } } ``` ########## fesod-cli/src/main/java/org/apache/fesod/cli/formatters/CsvFormatter.java: ########## @@ -0,0 +1,79 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.fesod.cli.formatters; + +import com.alibaba.fastjson2.JSONArray; +import com.alibaba.fastjson2.JSONObject; +import java.io.IOException; +import java.io.StringWriter; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.Map; +import org.apache.commons.csv.CSVFormat; +import org.apache.commons.csv.CSVPrinter; + +/** + * CSV formatter implementation + */ +public class CsvFormatter implements OutputFormatter { + + @Override + public String format(Map<String, Object> data) { + try { + StringWriter out = new StringWriter(); + CSVPrinter printer = new CSVPrinter(out, CSVFormat.DEFAULT); + + Object dataObj = data.get("data"); + + if (dataObj instanceof JSONArray) { + JSONArray array = (JSONArray) dataObj; + + for (int i = 0; i < array.size(); i++) { + Object item = array.get(i); + + if (item instanceof JSONObject) { + JSONObject obj = (JSONObject) item; + printer.printRecord(obj.values()); + } + } + } + + printer.close(); Review Comment: The CSVPrinter is explicitly closed but the StringWriter is not. While StringWriter doesn't hold external resources that need cleanup, for consistency and best practices, consider using try-with-resources for both the StringWriter and CSVPrinter. ########## fesod-cli/src/main/java/org/apache/fesod/cli/config/ConfigLoader.java: ########## @@ -0,0 +1,92 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.fesod.cli.config; + +import java.io.InputStream; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; +import org.yaml.snakeyaml.Yaml; + +/** + * Configuration loader + */ +public class ConfigLoader { + + private static final String USER_CONFIG_PATH = System.getProperty("user.home") + "/.fesod/config.yaml"; + + public CliConfig loadDefault() { + // 1. Try user home config (~/.fesod/config.yaml) + Path userConfig = Paths.get(USER_CONFIG_PATH); + if (Files.exists(userConfig)) { + System.out.println("Loading config from: " + userConfig); + return loadFromFile(userConfig); + } + + // 2. Try FESOD_HOME/conf/default-config.yaml + String fesodHome = System.getenv("FESOD_HOME"); + if (fesodHome != null && !fesodHome.isEmpty()) { + Path installConfig = Paths.get(fesodHome, "conf", "default-config.yaml"); + if (Files.exists(installConfig)) { + System.out.println("Loading config from: " + installConfig); + return loadFromFile(installConfig); + } + } + + // 3. Try relative path: conf/default-config.yaml + Path relativeConfig = Paths.get("conf", "default-config.yaml"); + if (Files.exists(relativeConfig)) { + System.out.println("Loading config from: " + relativeConfig.toAbsolutePath()); + return loadFromFile(relativeConfig); + } + + // 4. Fallback to embedded config in JAR + System.out.println("Loading default config from JAR"); + return createDefaultConfig(); + } + + public CliConfig loadFromFile(Path configPath) { + try { + try (InputStream is = Files.newInputStream(configPath)) { + Yaml yaml = new Yaml(); + CliConfig config = yaml.loadAs(is, CliConfig.class); + ConfigValidator.validate(config); Review Comment: The snakeyaml version 2.2 is specified, but this version uses the default constructor which can have security implications if untrusted YAML is loaded. Consider using SafeConstructor or newer versions with safer defaults to prevent arbitrary code execution through YAML deserialization attacks. ########## fesod-cli/src/main/java/org/apache/fesod/cli/FesodCli.java: ########## @@ -0,0 +1,96 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.fesod.cli; + +import org.apache.fesod.cli.commands.ConvertCommand; +import org.apache.fesod.cli.commands.InfoCommand; +import org.apache.fesod.cli.commands.ReadCommand; +import org.apache.fesod.cli.commands.VersionCommand; +import org.apache.fesod.cli.commands.WriteCommand; +import org.apache.fesod.cli.exception.CliException; +import picocli.CommandLine; +import picocli.CommandLine.Command; +import picocli.CommandLine.Model.CommandSpec; +import picocli.CommandLine.Option; +import picocli.CommandLine.Spec; + +@Command( + name = "fesod-cli", + mixinStandardHelpOptions = true, + version = {"Apache Fesod CLI", "Java Runtime: ${java.version}", "OS: ${os.name} ${os.arch}"}, Review Comment: There is an extra space in the version string. It should be "Java Runtime: ${java.version}" without the double space before the variable. ```suggestion version = {"Apache Fesod CLI", "Java Runtime: ${java.version}", "OS: ${os.name} ${os.arch}"}, ``` ########## fesod-cli/src/main/java/org/apache/fesod/cli/formatters/CsvFormatter.java: ########## @@ -0,0 +1,79 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.fesod.cli.formatters; + +import com.alibaba.fastjson2.JSONArray; +import com.alibaba.fastjson2.JSONObject; +import java.io.IOException; +import java.io.StringWriter; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.Map; +import org.apache.commons.csv.CSVFormat; +import org.apache.commons.csv.CSVPrinter; + +/** + * CSV formatter implementation + */ +public class CsvFormatter implements OutputFormatter { + + @Override + public String format(Map<String, Object> data) { + try { + StringWriter out = new StringWriter(); + CSVPrinter printer = new CSVPrinter(out, CSVFormat.DEFAULT); + + Object dataObj = data.get("data"); + + if (dataObj instanceof JSONArray) { + JSONArray array = (JSONArray) dataObj; + + for (int i = 0; i < array.size(); i++) { + Object item = array.get(i); + + if (item instanceof JSONObject) { + JSONObject obj = (JSONObject) item; + printer.printRecord(obj.values()); + } Review Comment: The CsvFormatter only formats data that has a "data" key in the map. If the map structure is different or the "data" key is missing, the CSV output will be empty without any warning or error. Consider adding validation to check if the "data" key exists and provide a meaningful error message if the data structure is invalid. ```suggestion if (data == null) { throw new RuntimeException("Invalid data structure for CSV formatting: input map is null."); } Object dataObj = data.get("data"); if (dataObj == null) { throw new RuntimeException("Invalid data structure for CSV formatting: missing 'data' key."); } if (!(dataObj instanceof JSONArray)) { throw new RuntimeException("Invalid data structure for CSV formatting: 'data' value must be a JSON array."); } JSONArray array = (JSONArray) dataObj; for (int i = 0; i < array.size(); i++) { Object item = array.get(i); if (item instanceof JSONObject) { JSONObject obj = (JSONObject) item; printer.printRecord(obj.values()); ``` ########## fesod-cli/src/main/java/org/apache/fesod/cli/commands/ConvertCommand.java: ########## @@ -0,0 +1,55 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.fesod.cli.commands; + +import java.nio.file.Path; +import java.nio.file.Paths; +import java.util.HashMap; +import java.util.Map; +import picocli.CommandLine.Command; +import picocli.CommandLine.Parameters; + +/** + * Convert command implementation + */ +@Command( + name = "convert", + description = "Convert spreadsheet between different formats", + mixinStandardHelpOptions = true) +public class ConvertCommand extends BaseCommand { + + @Parameters(index = "0", description = "Input file path", paramLabel = "<input>") + private String inputFile; + + @Parameters(index = "1", description = "Output file path", paramLabel = "<output>") + private String outputFile; + + @Override + protected void execute() { + Path input = Paths.get(inputFile); + Path output = Paths.get(outputFile); + + Map<String, Object> options = new HashMap<String, Object>(); + + processor.convert(input, output, options); + + getOut().println("✓ Conversion completed: " + inputFile + " → " + outputFile); Review Comment: There is an extra space in the success message. It should be "Conversion completed:" without the double space before the input file path. ```suggestion getOut().println("✓ Conversion completed: " + inputFile + " → " + outputFile); ``` ########## fesod-cli/src/main/java/org/apache/fesod/cli/core/sheet/SheetConverter.java: ########## @@ -0,0 +1,48 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.fesod.cli.core.sheet; + +import java.nio.file.Path; +import java.util.ArrayList; +import java.util.List; +import java.util.Map; +import org.apache.fesod.sheet.FesodSheet; + +/** + * Sheet format converter + */ +public class SheetConverter { + + public void convert(Path inputPath, Path outputPath, Map<String, Object> options) { + List<Map<Integer, String>> result = + FesodSheet.read(inputPath.toFile()).headRowNumber(0).sheet(0).doReadSync(); + + List<List<String>> allData = new ArrayList<List<String>>(); + for (Map<Integer, String> rowData : result) { + List<String> row = new ArrayList<String>(); + for (String value : rowData.values()) { + row.add(value); + } + allData.add(row); + } + + FesodSheet.write(outputPath.toFile()).sheet("Sheet1").doWrite(allData); + } Review Comment: The SheetConverter.convert() method only converts the first sheet (sheet index 0). If the input file has multiple sheets, only the first sheet will be converted, and the others will be lost. Consider adding an option to specify which sheet to convert or convert all sheets by default to avoid data loss. ########## fesod-cli/src/main/java/org/apache/fesod/cli/commands/WriteCommand.java: ########## @@ -0,0 +1,82 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.fesod.cli.commands; + +import com.alibaba.fastjson2.JSON; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.util.HashMap; +import java.util.Map; +import picocli.CommandLine.Command; +import picocli.CommandLine.Option; +import picocli.CommandLine.Parameters; + +/** + * Write command implementation + */ +@Command(name = "write", description = "Write data from JSON/CSV to spreadsheet", mixinStandardHelpOptions = true) +public class WriteCommand extends BaseCommand { + + @Parameters(index = "0", description = "Input data file (JSON/CSV)", paramLabel = "<input>") + private String inputFile; + + @Parameters(index = "1", description = "Output spreadsheet file", paramLabel = "<output>") + private String outputFile; + + @Option( + names = {"--input-format"}, + description = "Input data format: json, csv (default: json)", + defaultValue = "json") + private String inputFormat; + + @Option( + names = {"--sheet-name"}, + description = "Sheet name (default: Sheet1)", + defaultValue = "Sheet1") + private String sheetName; + + @Override + protected void execute() { + try { + Path input = Paths.get(inputFile); + Path output = Paths.get(outputFile); + Review Comment: The WriteCommand does not validate whether the input file exists before attempting to read it. If the input file doesn't exist, the error message may not be clear. Consider adding explicit file existence validation with a clear error message before attempting to read the file. ```suggestion if (!Files.exists(input)) { throw new RuntimeException("Input file does not exist: " + inputFile); } if (!Files.isRegularFile(input)) { throw new RuntimeException("Input path is not a regular file: " + inputFile); } if (!Files.isReadable(input)) { throw new RuntimeException("Input file is not readable: " + inputFile); } ``` ########## fesod-cli/src/main/java/org/apache/fesod/cli/config/ConfigLoader.java: ########## @@ -0,0 +1,92 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.fesod.cli.config; + +import java.io.InputStream; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; +import org.yaml.snakeyaml.Yaml; + +/** + * Configuration loader + */ +public class ConfigLoader { + + private static final String USER_CONFIG_PATH = System.getProperty("user.home") + "/.fesod/config.yaml"; + + public CliConfig loadDefault() { + // 1. Try user home config (~/.fesod/config.yaml) + Path userConfig = Paths.get(USER_CONFIG_PATH); + if (Files.exists(userConfig)) { + System.out.println("Loading config from: " + userConfig); + return loadFromFile(userConfig); + } + + // 2. Try FESOD_HOME/conf/default-config.yaml + String fesodHome = System.getenv("FESOD_HOME"); + if (fesodHome != null && !fesodHome.isEmpty()) { + Path installConfig = Paths.get(fesodHome, "conf", "default-config.yaml"); + if (Files.exists(installConfig)) { + System.out.println("Loading config from: " + installConfig); + return loadFromFile(installConfig); + } + } + + // 3. Try relative path: conf/default-config.yaml + Path relativeConfig = Paths.get("conf", "default-config.yaml"); + if (Files.exists(relativeConfig)) { + System.out.println("Loading config from: " + relativeConfig.toAbsolutePath()); + return loadFromFile(relativeConfig); + } + + // 4. Fallback to embedded config in JAR + System.out.println("Loading default config from JAR"); + return createDefaultConfig(); Review Comment: When catching exceptions, the error message is printed to System.out.println() instead of System.err.println(). Configuration loading errors should be sent to standard error. Consider changing these to System.err.println() for proper error handling. ########## fesod-cli/src/main/java/org/apache/fesod/cli/commands/ReadCommand.java: ########## @@ -0,0 +1,96 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.fesod.cli.commands; + +import java.nio.file.Path; +import java.nio.file.Paths; +import java.util.HashMap; +import java.util.Map; +import org.apache.fesod.cli.formatters.FormatterFactory; +import org.apache.fesod.cli.formatters.OutputFormatter; +import picocli.CommandLine.Command; +import picocli.CommandLine.Option; +import picocli.CommandLine.Parameters; + +/** + * Read command implementation + */ +@Command( + name = "read", + description = "Read spreadsheet data and output in specified format", + mixinStandardHelpOptions = true) +public class ReadCommand extends BaseCommand { + + @Parameters(index = "0", description = "Input file path", paramLabel = "<file>") + private String inputFile; + + @Option( + names = {"--format", "-f"}, + description = "Output format: json, csv (default: json)", + defaultValue = "json") + private String format; + + @Option( + names = {"--sheet", "-s"}, + description = "Sheet name or index (default: 0)", + paramLabel = "<name|index>") + private String sheet; + + @Option( + names = {"--output", "-o"}, + description = "Output file path (default: stdout)", + paramLabel = "<file>") + private String outputFile; + + @Option( + names = {"--all"}, + description = "Read all sheets") + private boolean readAll; + + @Override + protected void execute() { + Path input = Paths.get(inputFile); Review Comment: The ReadCommand does not validate whether the input file exists before attempting to read it. If the input file doesn't exist, the error message from the FileProcessException may not be as clear as a direct "File not found" message. Consider adding explicit file existence validation before processing. ########## fesod-cli/src/main/java/org/apache/fesod/cli/commands/WriteCommand.java: ########## @@ -0,0 +1,82 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.fesod.cli.commands; + +import com.alibaba.fastjson2.JSON; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.util.HashMap; +import java.util.Map; +import picocli.CommandLine.Command; +import picocli.CommandLine.Option; +import picocli.CommandLine.Parameters; + +/** + * Write command implementation + */ +@Command(name = "write", description = "Write data from JSON/CSV to spreadsheet", mixinStandardHelpOptions = true) +public class WriteCommand extends BaseCommand { + + @Parameters(index = "0", description = "Input data file (JSON/CSV)", paramLabel = "<input>") + private String inputFile; + + @Parameters(index = "1", description = "Output spreadsheet file", paramLabel = "<output>") + private String outputFile; + + @Option( + names = {"--input-format"}, + description = "Input data format: json, csv (default: json)", + defaultValue = "json") + private String inputFormat; + + @Option( + names = {"--sheet-name"}, + description = "Sheet name (default: Sheet1)", + defaultValue = "Sheet1") + private String sheetName; + + @Override + protected void execute() { + try { + Path input = Paths.get(inputFile); + Path output = Paths.get(outputFile); + + String content = new String(Files.readAllBytes(input), "UTF-8"); + Map<String, Object> data = new HashMap<String, Object>(); + + if ("json".equalsIgnoreCase(inputFormat)) { + data.put("data", JSON.parse(content)); + } else { + throw new UnsupportedOperationException("CSV input format not yet implemented"); + } + + Map<String, Object> options = new HashMap<String, Object>(); + options.put("sheetName", sheetName); + + processor.write(data, output, options); + + getOut().println("✓ Data written to: " + outputFile); + + } catch (Exception e) { + throw new RuntimeException("Failed to write data: " + e.getMessage(), e); Review Comment: There is an extra space in the error message. It should be "Failed to write data:" without the double space before the error message. ```suggestion throw new RuntimeException("Failed to write data: " + e.getMessage(), e); ``` ########## fesod-cli/src/main/java/org/apache/fesod/cli/core/sheet/SheetProcessor.java: ########## @@ -0,0 +1,137 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.fesod.cli.core.sheet; + +import java.nio.file.Path; +import java.util.ArrayList; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; +import org.apache.fesod.cli.core.DocumentProcessor; +import org.apache.fesod.cli.exception.FileProcessException; +import org.apache.fesod.sheet.ExcelReader; +import org.apache.fesod.sheet.FesodSheet; +import org.apache.fesod.sheet.read.metadata.ReadSheet; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * Sheet document processor implementation + */ +public class SheetProcessor implements DocumentProcessor { + + private static final Logger log = LoggerFactory.getLogger(SheetProcessor.class); + + private final SheetReader reader; + private final SheetWriter writer; + private final SheetConverter converter; + + public SheetProcessor() { + this.reader = new SheetReader(); + this.writer = new SheetWriter(); + this.converter = new SheetConverter(); + } + + @Override + public Map<String, Object> read(Path inputPath, Map<String, Object> options) { + log.info("Reading spreadsheet from: {}", inputPath); + + try { + Integer sheetIndex = (Integer) options.get("sheetIndex"); + String sheetName = (String) options.get("sheetName"); + Boolean readAll = (Boolean) options.get("readAll"); + if (readAll == null) { + readAll = false; + } + + return reader.read(inputPath, sheetIndex, sheetName, readAll); + + } catch (Exception e) { + throw new FileProcessException("Failed to read spreadsheet: " + e.getMessage(), e); Review Comment: There is an extra space in the error message. It should be "Failed to read spreadsheet:" without the double space before the error message. ```suggestion throw new FileProcessException("Failed to read spreadsheet: " + e.getMessage(), e); ``` ########## fesod-cli/src/main/java/org/apache/fesod/cli/commands/ConvertCommand.java: ########## @@ -0,0 +1,55 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.fesod.cli.commands; + +import java.nio.file.Path; +import java.nio.file.Paths; +import java.util.HashMap; +import java.util.Map; +import picocli.CommandLine.Command; +import picocli.CommandLine.Parameters; + +/** + * Convert command implementation + */ +@Command( + name = "convert", + description = "Convert spreadsheet between different formats", + mixinStandardHelpOptions = true) +public class ConvertCommand extends BaseCommand { + + @Parameters(index = "0", description = "Input file path", paramLabel = "<input>") + private String inputFile; + + @Parameters(index = "1", description = "Output file path", paramLabel = "<output>") + private String outputFile; + + @Override + protected void execute() { + Path input = Paths.get(inputFile); + Path output = Paths.get(outputFile); + Review Comment: The ConvertCommand does not validate whether the input and output files have different paths. If a user accidentally specifies the same path for both input and output, the original file could be overwritten before it's fully read, leading to data corruption or loss. Consider adding validation to prevent input and output files from being the same. ```suggestion if (input.toAbsolutePath().normalize().equals(output.toAbsolutePath().normalize())) { throw new IllegalArgumentException( "Input and output file paths must be different: " + inputFile); } ``` ########## fesod-cli/src/main/java/org/apache/fesod/cli/commands/WriteCommand.java: ########## @@ -0,0 +1,82 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.fesod.cli.commands; + +import com.alibaba.fastjson2.JSON; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.util.HashMap; +import java.util.Map; +import picocli.CommandLine.Command; +import picocli.CommandLine.Option; +import picocli.CommandLine.Parameters; + +/** + * Write command implementation + */ +@Command(name = "write", description = "Write data from JSON/CSV to spreadsheet", mixinStandardHelpOptions = true) +public class WriteCommand extends BaseCommand { + + @Parameters(index = "0", description = "Input data file (JSON/CSV)", paramLabel = "<input>") + private String inputFile; + + @Parameters(index = "1", description = "Output spreadsheet file", paramLabel = "<output>") + private String outputFile; + + @Option( + names = {"--input-format"}, + description = "Input data format: json, csv (default: json)", Review Comment: There is an extra space in the description. It should be "Input data format: json, csv (default: json)" without the double space before "json". ```suggestion description = "Input data format: json, csv (default: json)", ``` ########## fesod-cli/src/main/java/org/apache/fesod/cli/formatters/CsvFormatter.java: ########## @@ -0,0 +1,79 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.fesod.cli.formatters; + +import com.alibaba.fastjson2.JSONArray; +import com.alibaba.fastjson2.JSONObject; +import java.io.IOException; +import java.io.StringWriter; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.Map; +import org.apache.commons.csv.CSVFormat; +import org.apache.commons.csv.CSVPrinter; + +/** + * CSV formatter implementation + */ +public class CsvFormatter implements OutputFormatter { + + @Override + public String format(Map<String, Object> data) { + try { + StringWriter out = new StringWriter(); + CSVPrinter printer = new CSVPrinter(out, CSVFormat.DEFAULT); + + Object dataObj = data.get("data"); + + if (dataObj instanceof JSONArray) { + JSONArray array = (JSONArray) dataObj; + + for (int i = 0; i < array.size(); i++) { + Object item = array.get(i); + + if (item instanceof JSONObject) { + JSONObject obj = (JSONObject) item; + printer.printRecord(obj.values()); + } + } + } Review Comment: The CsvFormatter does not output CSV headers. When formatting JSON objects to CSV, the column headers (keys from the JSONObject) are not included in the output, making the CSV file difficult to interpret. Consider adding a header row with the keys from the first JSONObject before outputting the data rows. ########## fesod-cli/src/main/java/org/apache/fesod/cli/FesodCli.java: ########## @@ -0,0 +1,96 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.fesod.cli; + +import org.apache.fesod.cli.commands.ConvertCommand; +import org.apache.fesod.cli.commands.InfoCommand; +import org.apache.fesod.cli.commands.ReadCommand; +import org.apache.fesod.cli.commands.VersionCommand; +import org.apache.fesod.cli.commands.WriteCommand; +import org.apache.fesod.cli.exception.CliException; +import picocli.CommandLine; +import picocli.CommandLine.Command; +import picocli.CommandLine.Model.CommandSpec; +import picocli.CommandLine.Option; +import picocli.CommandLine.Spec; + +@Command( + name = "fesod-cli", + mixinStandardHelpOptions = true, + version = {"Apache Fesod CLI", "Java Runtime: ${java.version}", "OS: ${os.name} ${os.arch}"}, + description = "Fast and Easy spreadsheet processing from the command line", + subcommands = { + ReadCommand.class, + WriteCommand.class, + ConvertCommand.class, + InfoCommand.class, + VersionCommand.class, + CommandLine.HelpCommand.class + }, + usageHelpAutoWidth = true, + footer = { + "", + "Examples:", + " fesod-cli read data.xlsx --format json", + " fesod-cli convert input.xls output.xlsx", + " fesod-cli info data.xlsx", + "", + "Documentation: https://fesod.apache.org/docs/cli";, Review Comment: There is an extra space in the footer. It should be "Documentation: https://fesod.apache.org/docs/cli"; without the double space before the URL. ```suggestion "Documentation: https://fesod.apache.org/docs/cli";, ``` ########## fesod-cli/src/main/java/org/apache/fesod/cli/formatters/XmlFormatter.java: ########## @@ -0,0 +1,127 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.fesod.cli.formatters; + +import com.alibaba.fastjson2.JSONArray; +import com.alibaba.fastjson2.JSONObject; +import java.io.IOException; +import java.io.StringWriter; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.Map; +import javax.xml.parsers.DocumentBuilder; +import javax.xml.parsers.DocumentBuilderFactory; +import javax.xml.parsers.ParserConfigurationException; +import javax.xml.transform.OutputKeys; +import javax.xml.transform.Transformer; +import javax.xml.transform.TransformerException; +import javax.xml.transform.TransformerFactory; +import javax.xml.transform.dom.DOMSource; +import javax.xml.transform.stream.StreamResult; + +/** + * XML formatter implementation + */ +public class XmlFormatter implements OutputFormatter { + + @Override + public String format(Map<String, Object> data) { + try { + DocumentBuilderFactory docFactory = DocumentBuilderFactory.newInstance(); + DocumentBuilder docBuilder = docFactory.newDocumentBuilder(); + org.w3c.dom.Document doc = docBuilder.newDocument(); + + // Create root element + org.w3c.dom.Element rootElement = doc.createElement("fesod-result"); + doc.appendChild(rootElement); + + // Convert data to XML + convertToXml(doc, rootElement, data); + + // Transform to string + TransformerFactory transformerFactory = TransformerFactory.newInstance(); + Transformer transformer = transformerFactory.newTransformer(); + transformer.setOutputProperty(OutputKeys.INDENT, "yes"); + transformer.setOutputProperty("{http://xml.apache.org/xslt}indent-amount";, "2"); + + StringWriter writer = new StringWriter(); + transformer.transform(new DOMSource(doc), new StreamResult(writer)); + + return writer.toString(); + + } catch (ParserConfigurationException | TransformerException e) { + throw new RuntimeException("Failed to format as XML: " + e.getMessage(), e); + } + } + + private void convertToXml(org.w3c.dom.Document doc, org.w3c.dom.Element parent, Map<String, Object> data) { + for (Map.Entry<String, Object> entry : data.entrySet()) { + String key = entry.getKey(); + Object value = entry.getValue(); + + org.w3c.dom.Element element = doc.createElement(key.replaceAll("[^a-zA-Z0-9]", "_")); Review Comment: The XmlFormatter replaces all non-alphanumeric characters in XML element names with underscores, which could lead to name collisions. For example, both "user-name" and "user_name" would become "user_name". Consider using a more robust sanitization strategy or maintaining a mapping to preserve uniqueness. ########## fesod-cli/src/main/java/org/apache/fesod/cli/formatters/XmlFormatter.java: ########## @@ -0,0 +1,127 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.fesod.cli.formatters; + +import com.alibaba.fastjson2.JSONArray; +import com.alibaba.fastjson2.JSONObject; +import java.io.IOException; +import java.io.StringWriter; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.Map; +import javax.xml.parsers.DocumentBuilder; +import javax.xml.parsers.DocumentBuilderFactory; +import javax.xml.parsers.ParserConfigurationException; +import javax.xml.transform.OutputKeys; +import javax.xml.transform.Transformer; +import javax.xml.transform.TransformerException; +import javax.xml.transform.TransformerFactory; +import javax.xml.transform.dom.DOMSource; +import javax.xml.transform.stream.StreamResult; + +/** + * XML formatter implementation + */ +public class XmlFormatter implements OutputFormatter { + + @Override + public String format(Map<String, Object> data) { + try { + DocumentBuilderFactory docFactory = DocumentBuilderFactory.newInstance(); + DocumentBuilder docBuilder = docFactory.newDocumentBuilder(); + org.w3c.dom.Document doc = docBuilder.newDocument(); + + // Create root element + org.w3c.dom.Element rootElement = doc.createElement("fesod-result"); + doc.appendChild(rootElement); + + // Convert data to XML + convertToXml(doc, rootElement, data); + + // Transform to string + TransformerFactory transformerFactory = TransformerFactory.newInstance(); + Transformer transformer = transformerFactory.newTransformer(); + transformer.setOutputProperty(OutputKeys.INDENT, "yes"); + transformer.setOutputProperty("{http://xml.apache.org/xslt}indent-amount";, "2"); + + StringWriter writer = new StringWriter(); + transformer.transform(new DOMSource(doc), new StreamResult(writer)); + + return writer.toString(); + + } catch (ParserConfigurationException | TransformerException e) { + throw new RuntimeException("Failed to format as XML: " + e.getMessage(), e); + } + } + + private void convertToXml(org.w3c.dom.Document doc, org.w3c.dom.Element parent, Map<String, Object> data) { + for (Map.Entry<String, Object> entry : data.entrySet()) { + String key = entry.getKey(); + Object value = entry.getValue(); + + org.w3c.dom.Element element = doc.createElement(key.replaceAll("[^a-zA-Z0-9]", "_")); + + if (value instanceof Map) { + convertToXml(doc, element, (Map<String, Object>) value); + } else if (value instanceof JSONArray) { + JSONArray array = (JSONArray) value; + for (int i = 0; i < array.size(); i++) { + Object item = array.get(i); + org.w3c.dom.Element itemElement = doc.createElement("item"); + + if (item instanceof JSONObject) { + convertToXml(doc, itemElement, (JSONObject) item); + } else { + itemElement.setTextContent(item != null ? item.toString() : ""); + } + + element.appendChild(itemElement); + } + } else { + element.setTextContent(value != null ? value.toString() : ""); + } + + parent.appendChild(element); + } + } + + private void convertToXml(org.w3c.dom.Document doc, org.w3c.dom.Element parent, JSONObject jsonObject) { Review Comment: Method XmlFormatter.convertToXml(..) could be confused with overloaded method [convertToXml](1), since dispatch depends on static types. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
