Add CSV validation warnings and example CSV files#204
Closed
Pranav-0440 wants to merge 2 commits intoeclipse-editdor:masterfrom
Closed
Add CSV validation warnings and example CSV files#204Pranav-0440 wants to merge 2 commits intoeclipse-editdor:masterfrom
Pranav-0440 wants to merge 2 commits intoeclipse-editdor:masterfrom
Conversation
Signed-off-by: Pranav-0440 <pranavghorpade61@gmail.com>
✅ Deploy Preview for editdor ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Signed-off-by: Pranav-0440 <pranavghorpade61@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR adds CSV validation warnings to detect invalid data types and Modbus entities during CSV import, addressing issue #134. However, it includes a critical unrelated change that removes core functionality from the ConvertTmDialog component, making this PR unsuitable for merging in its current state.
Changes:
- Added CSV validation system that warns users about invalid
typeandmodbus:entityvalues while still allowing data import - Added example CSV files (valid.csv and invalid.csv) with documentation to demonstrate the validation system
- Modified ConvertTmDialog component (appears to be unintentional breaking change)
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils/parser.ts | Added CSV validation warnings for invalid types and modbus entities; simplified copyright header |
| src/utils/parser.test.ts | Updated all parseCsv test calls to destructure new return format with warnings |
| src/components/App/CreateTd.tsx | Added display of CSV validation warnings to users during file import |
| src/components/Dialogs/ConvertTmDialog.tsx | CRITICAL: Removed all Thing Model conversion functionality, leaving broken implementation |
| test/csv-examples.test.ts | Added demonstration test file using console.log instead of proper assertions |
| doc/examples/csv/valid.csv | Added example CSV with valid data |
| doc/examples/csv/invalid.csv | Added example CSV with intentional validation errors |
| doc/examples/csv/README.md | Added documentation for example files with inaccurate descriptions |
Comments suppressed due to low confidence (9)
src/utils/parser.ts:135
- Warnings are collected for all parsed rows before filtering out empty rows. This means warnings will be generated with incorrect row numbers if there are empty rows in the CSV, since the row numbers are calculated based on the unfiltered data but users will see the filtered data. The filtering should happen before warning collection, or the row numbers should be adjusted after filtering.
res.data.forEach((row, index) => {
const rowNum = index + 2;
if (row.type && !VALID_TYPES.includes(row.type)) {
warnings.push({
row: rowNum,
column: "type",
message: `Invalid type "${row.type}"`,
});
}
if (row["modbus:entity"]) {
const entityLower = row["modbus:entity"].toLowerCase();
const validEntityLower = VALID_MODBUS_ENTITIES.map((e) => e.toLowerCase());
if (!validEntityLower.includes(entityLower)) {
warnings.push({
row: rowNum,
column: "modbus:entity",
message: `Invalid modbus entity "${row["modbus:entity"]}"`,
});
}
}
});
return {
data: res.data.filter((row) =>
Object.values(row).some((v) => v !== "" && v != null)
),
warnings,
};
src/utils/parser.ts:5
- The copyright header in this file has been shortened and is now inconsistent with all other files in the codebase. All other source files in src/ use the full copyright header that includes "See the NOTICE file(s) distributed with this work for additional information regarding copyright ownership" and the full license text. This file should use the same format as other files for consistency.
/********************************************************************************
* Copyright (c) 2025 Contributors to the Eclipse Foundation
*
* SPDX-License-Identifier: EPL-2.0 OR W3C-20150513
********************************************************************************/
doc/examples/csv/README.md:55
- The documentation states "The validation is case-insensitive for Modbus entities" but this contradicts the examples shown above which indicate that case sensitivity matters. Lines 25 and 28 list "holdingregister" and "coil" (lowercase) as invalid because they should be "HoldingRegister" and "Coil". If validation is truly case-insensitive, these should not be listed as errors.
Note: The validation is case-insensitive for Modbus entities, so `coil`, `Coil`, and `COIL` are all treated as equivalent.
src/components/Dialogs/ConvertTmDialog.tsx:32
- The warnings state is initialized but never populated. There is no call to setWarnings anywhere in this component, which means the warning display UI on lines 73-84 will never be shown. If this is meant to display CSV warnings from the parser, the warnings need to be passed to this component or retrieved from context.
const [warnings, setWarnings] = useState<CsvWarning[]>([]);
src/components/Dialogs/ConvertTmDialog.tsx:21
- The CsvWarning type is imported but this dialog is for Thing Model to Thing Description conversion, not CSV import. This import appears to be misplaced and suggests confusion about the purpose of this component. CSV warnings should be displayed in the CSV import dialog (CreateTd.tsx), not here.
import { CsvWarning } from "../../utils/parser";
doc/examples/csv/README.md:28
- The documentation claims that rows 2 and 5 have invalid modbus:entity values due to case sensitivity ("holdingregister" and "coil" should be "HoldingRegister" and "Coil"). However, the validation code (lines 117-127 in parser.ts) performs case-insensitive matching by converting both the input and valid entities to lowercase. This means "holdingregister" will match "HoldingRegister" and "coil" will match "Coil", so these should NOT generate warnings. The expected warnings listed in this documentation do not match the actual behavior of the validation code.
A CSV file with intentional validation errors to demonstrate the warning system. When imported, this file will trigger the following warnings:
1. **Row 2, type**: `number123` is invalid (should be `number`, `string`, or `boolean`)
2. **Row 2, modbus:entity**: `holdingregister` is invalid (case-sensitive, should be `HoldingRegister`)
3. **Row 3, modbus:entity**: `InvalidRegister` is not a recognized Modbus entity
4. **Row 4, type**: `invalid_type` is not a valid type
5. **Row 5, modbus:entity**: `coil` is invalid (case-sensitive, should be `Coil`)
src/utils/parser.ts:88
- The hasHeaders parameter is declared but never used in the function body. The Papa.parse configuration always sets header: true regardless of this parameter value. Either remove this parameter or use it to conditionally set the header option in the Papa.parse configuration.
export const parseCsv = (
csvContent: string,
hasHeaders: boolean = true
): { data: CsvData[]; warnings: CsvWarning[] } => {
src/utils/parser.ts:127
- Creating the validEntityLower array on every iteration is inefficient. This mapping should be done once outside the forEach loop to avoid unnecessary repeated computation for each row.
if (row["modbus:entity"]) {
const entityLower = row["modbus:entity"].toLowerCase();
const validEntityLower = VALID_MODBUS_ENTITIES.map((e) => e.toLowerCase());
if (!validEntityLower.includes(entityLower)) {
warnings.push({
row: rowNum,
column: "modbus:entity",
message: `Invalid modbus entity "${row["modbus:entity"]}"`,
});
}
}
test/csv-examples.test.ts:69
- This test file uses console.log for output instead of proper test assertions, which means it won't integrate properly with the Vitest test framework. It should use describe and test blocks with expect assertions like the other test files in the project. Without proper assertions, this test will always pass even if validation fails.
import { parseCsv, mapCsvToProperties } from "../src/utils/parser";
import * as fs from "fs";
import * as path from "path";
import { fileURLToPath } from "url";
const __filename = fileURLToPath(import.meta.url);
const __dirname = path.dirname(__filename);
console.log("=== Testing CSV Examples ===\n");
// Test valid.csv
console.log("1. Testing valid.csv:");
const validCsv = fs.readFileSync(
path.join(__dirname, "../doc/examples/csv/valid.csv"),
"utf-8"
);
const validResult = parseCsv(validCsv, true);
console.log(` - Rows parsed: ${validResult.data.length}`);
console.log(` - Warnings: ${validResult.warnings.length}`);
if (validResult.warnings.length > 0) {
console.log(" FAIL: Expected no warnings but got:");
validResult.warnings.forEach((w) =>
console.log(` Row ${w.row}, ${w.column}: ${w.message}`)
);
} else {
console.log(" PASS: No warnings as expected");
}
// Try to map properties
try {
const properties = mapCsvToProperties(validResult.data);
console.log(` - Properties created: ${Object.keys(properties).length}`);
console.log(" PASS: Properties mapped successfully");
} catch (err) {
console.log(` FAIL: ${(err as Error).message}`);
}
console.log("\n2. Testing invalid.csv:");
const invalidCsv = fs.readFileSync(
path.join(__dirname, "../doc/examples/csv/invalid.csv"),
"utf-8"
);
const invalidResult = parseCsv(invalidCsv, true);
console.log(` - Rows parsed: ${invalidResult.data.length}`);
console.log(` - Warnings: ${invalidResult.warnings.length}`);
if (invalidResult.warnings.length === 0) {
console.log(" FAIL: Expected warnings but got none");
} else {
console.log(" PASS: Warnings detected as expected:");
invalidResult.warnings.forEach((w) =>
console.log(` Row ${w.row}, ${w.column}: ${w.message}`)
);
}
// Try to map properties - should still work despite warnings
try {
const properties = mapCsvToProperties(invalidResult.data);
console.log(` - Properties created: ${Object.keys(properties).length}`);
console.log(" PASS: Properties mapped successfully despite warnings");
} catch (err) {
console.log(` FAIL: ${(err as Error).message}`);
}
console.log("\n=== Summary ===");
console.log("All CSV example files tested successfully!");
console.log(
"The validation system correctly identifies issues while still allowing data import."
);
Comment on lines
45
to
53
| const handleGenerateTd = () => { | ||
| const selections = getSelectedAffordances(affordanceElements); | ||
|
|
||
| const newTD = processConversionTMtoTD( | ||
| context.offlineTD, | ||
| placeholderValues, | ||
| selections.properties, | ||
| selections.actions, | ||
| selections.events, | ||
| {}, | ||
| [], | ||
| [], | ||
| [], | ||
| versionInput | ||
| ); |
There was a problem hiding this comment.
Passing empty objects and arrays to processConversionTMtoTD will not produce a correct Thing Description. The original implementation extracted placeholders, affordances, and required fields from the Thing Model. Without this logic, the conversion will fail or produce incorrect output.
Comment on lines
1
to
101
| @@ -22,77 +15,40 @@ import ediTDorContext from "../../context/ediTDorContext"; | |||
| import DialogTemplate from "./DialogTemplate"; | |||
| import { | |||
| processConversionTMtoTD, | |||
| extractPlaceholders, | |||
| isVersionValid, | |||
| } from "../../services/operations"; | |||
| import TmInputForm from "../base/TmInputForm"; | |||
| import TextField from "../base/TextField"; | |||
| import { CsvWarning } from "../../utils/parser"; | |||
|
|
|||
| export interface ConvertTmDialogRef { | |||
| openModal: () => void; | |||
| close: () => void; | |||
| } | |||
|
|
|||
| const ConvertTmDialog = forwardRef<ConvertTmDialogRef>((props, ref) => { | |||
| const context: IEdiTDorContext = useContext(ediTDorContext); | |||
| const td: string = context.offlineTD; | |||
| const [htmlInputs, setHtmlInputs] = useState<JSX.Element[]>([]); | |||
| const [display, setDisplay] = useState<boolean>(() => { | |||
| return false; | |||
| }); | |||
| const [affordanceElements, setAffordanceElements] = useState<JSX.Element[]>( | |||
| [] | |||
| ); | |||
| const [placeholderValues, setPlaceholderValues] = useState< | |||
| Record<string, string> | |||
| >({}); | |||
| const context = useContext(ediTDorContext); | |||
|
|
|||
| const [validVersion, setValidVersion] = useState<boolean>(false); | |||
| const [versionInput, setVersionInput] = useState<string>(""); | |||
| const [display, setDisplay] = useState(false); | |||
| const [warnings, setWarnings] = useState<CsvWarning[]>([]); | |||
| const [validVersion, setValidVersion] = useState(false); | |||
| const [versionInput, setVersionInput] = useState(""); | |||
|
|
|||
| useEffect(() => { | |||
| setValidVersion(isVersionValid(context.parsedTD)); | |||
| }, [context.parsedTD]); | |||
|
|
|||
| useEffect(() => { | |||
| setHtmlInputs(createHtmlInputs(context.offlineTD)); | |||
| setAffordanceElements(createAffordanceElements(context.offlineTD)); | |||
| }, [context.offlineTD]); | |||
|
|
|||
| useEffect(() => { | |||
| if (td) { | |||
| const placeholders = extractPlaceholders(td); | |||
| const initialValues = placeholders.reduce<Record<string, string>>( | |||
| (acc, key) => { | |||
| acc[key] = ""; | |||
| return acc; | |||
| }, | |||
| {} | |||
| ); | |||
| setPlaceholderValues(initialValues); | |||
| } | |||
| }, [td]); | |||
|
|
|||
| useImperativeHandle(ref, () => ({ | |||
| openModal: () => setDisplay(true), | |||
| close: () => setDisplay(false), | |||
| })); | |||
|
|
|||
| const handleFieldChange = (placeholder: string, value: string) => { | |||
| setPlaceholderValues((prev) => ({ | |||
| ...prev, | |||
| [placeholder]: value, | |||
| })); | |||
| }; | |||
| const handleGenerateTd = () => { | |||
| const selections = getSelectedAffordances(affordanceElements); | |||
|
|
|||
| const newTD = processConversionTMtoTD( | |||
| context.offlineTD, | |||
| placeholderValues, | |||
| selections.properties, | |||
| selections.actions, | |||
| selections.events, | |||
| {}, | |||
| [], | |||
| [], | |||
| [], | |||
| versionInput | |||
| ); | |||
| const resultJson = JSON.stringify(newTD, null, 2); | |||
| @@ -103,190 +59,43 @@ const ConvertTmDialog = forwardRef<ConvertTmDialogRef>((props, ref) => { | |||
| ); | |||
| }; | |||
|
|
|||
| const handleVersionInputChange = ( | |||
| e: React.ChangeEvent<HTMLInputElement> | |||
| ): void => { | |||
| const value = e.target.value; | |||
| const trimmedValue = value.trim(); | |||
| setVersionInput(trimmedValue); | |||
| }; | |||
|
|
|||
| if (!display) return null; | |||
|
|
|||
| if (display) { | |||
| return ReactDOM.createPortal( | |||
| <DialogTemplate | |||
| onHandleEventLeftButton={() => setDisplay(false)} | |||
| onHandleEventRightButton={handleGenerateTd} | |||
| rightButton={"Generate TD"} | |||
| title={"Generate TD From TM"} | |||
| description={"Please provide values to switch the placeholders with."} | |||
| > | |||
| <> | |||
| <TmInputForm | |||
| inputValues={placeholderValues} | |||
| onValueChange={handleFieldChange} | |||
| return ReactDOM.createPortal( | |||
| <DialogTemplate | |||
| onHandleEventLeftButton={() => setDisplay(false)} | |||
| onHandleEventRightButton={handleGenerateTd} | |||
| rightButton={"Generate TD"} | |||
| title={"Generate TD From TM"} | |||
| description={"CSV conversion completed"} | |||
| > | |||
| <> | |||
| {warnings.length > 0 && ( | |||
| <div className="mb-4 rounded bg-yellow-900 p-3 text-yellow-200"> | |||
| <h3 className="font-bold">CSV Import Warnings</h3> | |||
| <ul className="list-disc pl-5"> | |||
| {warnings.map((w, i) => ( | |||
| <li key={i}> | |||
| Row {w.row}, {w.column}: {w.message} | |||
| </li> | |||
| ))} | |||
| </ul> | |||
| </div> | |||
| )} | |||
|
|
|||
| {!validVersion && ( | |||
| <TextField | |||
| label="TD instance version" | |||
| onChange={(e) => setVersionInput(e.target.value.trim())} | |||
| value={versionInput} | |||
| placeholder="ex: 1.0.0" | |||
| /> | |||
|
|
|||
| {!validVersion && ( | |||
| <TextField | |||
| label="TD instance version" | |||
| id="instance" | |||
| autoFocus={true} | |||
| onChange={handleVersionInputChange} | |||
| placeholder="ex: 1.0.0" | |||
| value={versionInput} | |||
| helperText="The Thing Model contains a version without instance key and corresponding value. If you leave this field empty it will automatic generate a instance value." | |||
| ></TextField> | |||
| )} | |||
| <h2 className="pb-2 pt-4 text-gray-400"> | |||
| Select/unselect the interaction affordances you would like to see in | |||
| the new TD. | |||
| </h2> | |||
|
|
|||
| <div className="affordances-container">{affordanceElements}</div> | |||
| </> | |||
| </DialogTemplate>, | |||
| document.getElementById("modal-root") as HTMLElement | |||
| ); | |||
| } | |||
|
|
|||
| return null; | |||
| )} | |||
| </> | |||
| </DialogTemplate>, | |||
| document.getElementById("modal-root") as HTMLElement | |||
| ); | |||
| }); | |||
|
|
|||
| function getSelectedAffordances(elements: JSX.Element[]) { | |||
| const result = { | |||
| properties: [] as string[], | |||
| actions: [] as string[], | |||
| events: [] as string[], | |||
| }; | |||
|
|
|||
| elements.forEach((element) => { | |||
| if (element.props.className.includes("form-checkbox")) { | |||
| const checkbox = document.getElementById( | |||
| element.props.children[0].props.id | |||
| ) as HTMLInputElement | null; | |||
| if (checkbox?.checked) { | |||
| const [type, name] = element.key?.toString().split("/") ?? []; | |||
|
|
|||
| if (type === "properties") result.properties.push(name); | |||
| else if (type === "actions") result.actions.push(name); | |||
| else if (type === "events") result.events.push(name); | |||
| } | |||
| } | |||
| }); | |||
|
|
|||
| return result; | |||
| } | |||
|
|
|||
| // Create affordance element remains similar to your original implementation | |||
| function createAffordanceElements(tmContent: string): JSX.Element[] { | |||
| try { | |||
| if (!tmContent) return []; | |||
| const parsed = JSON.parse(tmContent); | |||
| const { properties, actions, events, requiredFields } = | |||
| extractAffordances(parsed); | |||
|
|
|||
| const propertyElements = createAffordanceHtml( | |||
| "properties", | |||
| properties, | |||
| requiredFields | |||
| ); | |||
| const actionElements = createAffordanceHtml( | |||
| "actions", | |||
| actions, | |||
| requiredFields | |||
| ); | |||
| const eventElements = createAffordanceHtml( | |||
| "events", | |||
| events, | |||
| requiredFields | |||
| ); | |||
|
|
|||
| return [...propertyElements, ...actionElements, ...eventElements]; | |||
| } catch (e) { | |||
| console.error("Error creating affordance elements:", e); | |||
| return []; | |||
| } | |||
| } | |||
|
|
|||
| const createHtmlInputs = (td: string): JSX.Element[] => { | |||
| try { | |||
| let htmlProperties: JSX.Element[] = []; | |||
| let htmlActions: JSX.Element[] = []; | |||
| let htmlEvents: JSX.Element[] = []; | |||
|
|
|||
| try { | |||
| const parsed = JSON.parse(td); | |||
|
|
|||
| const { properties, actions, events, requiredFields } = | |||
| extractAffordances(parsed); | |||
|
|
|||
| htmlProperties = createAffordanceHtml( | |||
| "properties", | |||
| properties, | |||
| requiredFields | |||
| ); | |||
| htmlActions = createAffordanceHtml("actions", actions, requiredFields); | |||
| htmlEvents = createAffordanceHtml("events", events, requiredFields); | |||
| } catch (ignored) {} | |||
|
|
|||
| return [...htmlProperties, ...htmlActions, ...htmlEvents]; | |||
| } catch (e) { | |||
| console.error("Error creating HTML inputs:", e); | |||
| return []; | |||
| } | |||
| }; | |||
|
|
|||
| function createAffordanceHtml( | |||
| affName: "properties" | "actions" | "events", | |||
| affContainer: string[], | |||
| requiredFields: { [k: string]: string[] } | |||
| ): JSX.Element[] { | |||
| return affContainer.map((aff) => { | |||
| const required = requiredFields[affName].includes(aff); | |||
| return ( | |||
| <div key={`${affName}/${aff}`} className="form-checkbox py-1 pl-2"> | |||
| <input | |||
| id={`${affName}/${aff}`} | |||
| className="form-checkbox-input" | |||
| type="checkbox" | |||
| value={`#${affName}/${aff}`} | |||
| disabled={required} | |||
| defaultChecked={true} | |||
| title={required ? "This field is required by the TM." : ""} | |||
| data-interaction={affName} | |||
| /> | |||
| <label | |||
| className="form-checkbox-label pl-2" | |||
| htmlFor={`${affName}/${aff}`} | |||
| >{`#${affName}/${aff}`}</label> | |||
| </div> | |||
| ); | |||
| }); | |||
| } | |||
|
|
|||
| function extractAffordances(parsed: any) { | |||
| const properties = Object.keys(parsed["properties"] || {}); | |||
| const actions = Object.keys(parsed["actions"] || {}); | |||
| const events = Object.keys(parsed["events"] || {}); | |||
| const requiredFields = { properties: [], actions: [], events: [] }; | |||
|
|
|||
| if (parsed["tm:required"]) { | |||
| for (const field of parsed["tm:required"]) { | |||
| if (field.startsWith("#properties/")) | |||
| // @ts-ignore | |||
| requiredFields["properties"].push(field.split("/")[1]); | |||
| else if (field.startsWith("#actions/")) | |||
| // @ts-ignore | |||
| requiredFields["actions"].push(field.split("/")[1]); | |||
| else if (field.startsWith("#events/")) | |||
| // @ts-ignore | |||
| requiredFields["events"].push(field.split("/")[1]); | |||
| } | |||
| } | |||
| return { properties, actions, events, requiredFields }; | |||
| } | |||
|
|
|||
| ConvertTmDialog.displayName = "ConvertTmDialog"; | |||
| export default ConvertTmDialog; | |||
There was a problem hiding this comment.
This dialog has been gutted of its core functionality. The original file contained logic for handling Thing Model to Thing Description conversion including placeholder replacement, affordance selection, and form handling. This refactored version removes all of that functionality and introduces a warnings state that is never populated (setWarnings is never called). This appears to be unrelated to the CSV validation feature and may have been included in this PR by mistake. The dialog now passes empty arrays to processConversionTMtoTD which will not work correctly.
Contributor
|
No @Pranav-0440, you should not recreated a new branch. Your work is on #195, please continue there. Closing PR. |
Contributor
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Recreated clean branch without unrelated formatting and line ending changes.
Adds CSV validation warnings and example CSV files as requested in #134.
Closes #134