Code Review Phase 6: Validation Engine

[jitu5]

Code Review Phase 6: Validation Engine

Purpose: Review the pipeline validation system — both real-time input validation (form-level) and pipeline-level validation (export-blocking). Uses the Strategy Pattern with a ValidatorRegistry.

Prerequisites: Phases 1, 3 (Types, Domain Layer)
Estimated time: 1.5 hours
Files: 16

---

Validation Flow

  User clicks "Export" or "View Code"
         │
         ▼
  ┌─────────────────────────────┐
  │   ValidatorRegistry.run()   │
  │                             │
  │  ┌───────────────────────┐  │
  │  │ EmptyNameValidator    │──┼──▶ error
  │  │ InvalidNameValidator  │──┼──▶ error
  │  │ DuplicateNameValidator│──┼──▶ error
  │  │ CircularDepValidator  │──┼──▶ error  ◀── uses PipelineGraph
  │  │ OrphanedNodeValidator │──┼──▶ warning
  │  │ OrphanedDatasetValid. │──┼──▶ warning
  │  │ MissingCodeValidator  │──┼──▶ warning
  │  │ MissingConfigValidator│──┼──▶ warning
  │  └───────────────────────┘  │
  └──────────────┬──────────────┘
                 │
        ┌────────┴────────┐
        ▼                 ▼
   Has errors?        No errors
   Show panel        Proceed to export

---

Files to Review (in order)

Type definitions

• src/utils/validation/types.ts — ValidationError, ValidationResult, InputValidationResult types (canonical source for ValidationError)

Input validation (real-time, form-level)

• src/utils/validation/inputValidation.ts — Real-time name validation, Python keyword blocklist

Strategy Pattern validators

• src/utils/validation/validators/CircularDependencyValidator.ts — Wraps PipelineGraph cycle detection
• src/utils/validation/validators/DuplicateNameValidator.ts — Case-insensitive name collision checking
• src/utils/validation/validators/EmptyNameValidator.ts — Checks for unnamed components
• src/utils/validation/validators/InvalidNameValidator.ts — Name format checking (snake_case for datasets)
• src/utils/validation/validators/MissingCodeValidator.ts — Warns about nodes without function code
• src/utils/validation/validators/MissingConfigValidator.ts — Warns about datasets without config
• src/utils/validation/validators/OrphanedNodeValidator.ts — Warns about unconnected nodes
• src/utils/validation/validators/OrphanedDatasetValidator.ts — Warns about unconnected datasets
• src/utils/validation/validators/index.ts — Registry factory (trimmed from 8 methods to 3)

Orchestration

• src/utils/validation/pipelineValidation.ts — Thin wrapper delegating to ValidatorRegistry
• src/utils/validation/index.ts — Barrel export

Tests

• src/utils/validation.test.ts — 29 tests: input validation (node names, dataset names, keywords)
• src/utils/validation/validators/validators.test.ts — 41 tests: all 8 Strategy Pattern validators

---

Key Concepts

• Strategy Pattern: each validator implements the Validator interface with validate(state: RootState): ValidationError[]
• ValidatorRegistry collects validators and runs them all, splitting results by severity (error vs warning)
• pipelineValidation.ts is a 27-line thin wrapper that delegates to the ValidatorRegistry — it is the single public API for pipeline validation
• ValidationError is defined in utils/validation/types.ts as the single canonical source (previously duplicated in types/kedro.ts)

---

Focus Areas

• pipelineValidation.ts is now a thin 27-line wrapper delegating to ValidatorRegistry. Verify it correctly passes through all errors and warnings.
• ValidatorRegistry was trimmed from 8 methods to 3 (factory, validateAll, singleton getter). Verify the public API is sufficient.
• validateNodeName allows spaces in names, but validateDatasetName requires strict snake_case — is this intentional and documented?
• getDefaultValidatorRegistry uses a module-level singleton — is this safe with HMR/testing?
• src/utils/validation.ts is a deprecated re-export shim. It is still imported by several files. Verify whether this shim should be removed and imports updated to ./validation/index directly.
• PYTHON_KEYWORDS is imported by infrastructure/export/helpers.ts — this cross-layer dependency couples validation to export. Evaluate whether PYTHON_KEYWORDS should live in constants/ instead.
• Each validator now uses a shared getConnectionsArray helper instead of duplicating connection mapping logic.
• Review validators.test.ts — do the 41 tests cover all 8 validators with both positive and negative cases?
• Review validation.test.ts — does it cover edge cases like empty strings, unicode, Python keywords?

---

Next: Phase 7 (Utilities & Hooks)

[ravi-kumar-pilla]

I am not sure if placing validation under utils is a right choice. Here we are not utilizing these functions in multiple places of the app but validation of the code or the validation engine itself is a core requirements of our application. What I want to say is validation engine is also a feature of the app and not just a helper function used in multiple places of the app. Like an infrastructure folder, this also needs a separate folder. src/validation

[ravi-kumar-pilla]

Some issues in the validation engine:

1. PYTHON_KEYWORDS Creates an Illegal Cross-Layer Dependency

Files:

• src/utils/validation/inputValidation.ts
• src/infrastructure/export/helpers.ts

Severity: High — architectural violation

// infrastructure/export/helpers.ts
import { PYTHON_KEYWORDS } from '../../utils/validation';

PYTHON_KEYWORDS is a data constant — a list of 36 strings. It lives in utils/validation/inputValidation.ts because that's where it was first needed. The infrastructure layer then imports it from there.

The dependency direction is wrong: infrastructure (export/helpers.ts) must not depend on the validation utilities layer (utils/validation). The correct dependency graph is:

infrastructure ──▶ constants    ✓
validation      ──▶ constants    ✓
infrastructure  ──▶ validation   ✗  (current, wrong direction)

PYTHON_KEYWORDS is a pure data constant with no logic. It belongs in src/constants/python.ts (or src/constants/index.ts). Both inputValidation.ts and helpers.ts would then import it from the shared constants layer with correct dependency direction.

As a secondary issue, the import goes through the deprecated validation.ts shim (import { PYTHON_KEYWORDS } from '../../utils/validation'), compounding the problem.

---

2. MissingConfigValidator Generates False-Positive Warnings for Non-File Datasets

File: src/utils/validation/validators/MissingConfigValidator.ts
Severity: High — incorrect validation behavior for real Kedro workflows

if (dataset.type !== 'memory' && !dataset.filepath?.trim()) {
  issues.push('filepath');
}

Only memory datasets are exempted from the filepath requirement. But the DatasetType enum contains numerous dataset types where a local filepath is architecturally meaningless:

Dataset type	Why filepath doesn't apply
api	Fetches from a URL, not a file path
gbq_table, gbq_query	BigQuery table/query identifiers
sql_table, sql_query	SQL connection + table name
spark_hive	Hive metastore table reference
tracking	MLflow tracking URI

A user who correctly configures a BigQuery dataset with type: 'gbq_table' and no filepath receives a spurious "Dataset 'X' is missing: filepath" warning. This warning:

1. Is technically incorrect for that dataset type
2. Creates noise that trains users to ignore the validation panel
3. Will block export if the export validation logic treats this warning as blocking (depends on downstream logic)

The exemption set must be expanded to include all non-file-based dataset types. A maintainable approach is a FILE_BASED_DATASET_TYPES constant (or an FILEPATH_EXEMPT_TYPES set) to make the exemptions explicit and maintainable as new dataset types are added.

---

3. sanitizeForPython Can Silently Return an Empty String

File: src/utils/validation/inputValidation.ts
Severity: Medium — correctness bug, no caller guard

export function sanitizeForPython(name: string): string {
  return name
    .trim()
    .toLowerCase()
    .replace(/\s+/g, '_')
    .replace(/[^a-z0-9_]/g, '')   // Remove all non-alphanumeric/underscore
    .replace(/^[0-9]+/, '')        // Strip leading digits
    .replace(/_+/g, '_')           // Collapse underscores
    .replace(/^_|_$/g, '');        // Strip leading/trailing underscores
}

For certain inputs, the chain produces an empty string:

'123'   → '123' → '' (after strip-leading-digits) → '' (final)
'___'   → '___' → '_' (after collapse) → '' (after strip underscores)
'@@@'   → '' (all invalid chars stripped) → '' (final)
'!!!'   → '' (same)

sanitizeForPython('') is not a valid Python identifier. Any code generation that uses the result without checking could produce malformed Python:

# Generated code with empty sanitized name:
def (raw_data: pd.DataFrame):  # SyntaxError

No caller currently validates the return value. Fix: add a guard at the end of the transformation chain (|| 'unnamed_function') or document the empty-string case and require callers to validate.

[ravi-kumar-pilla]

Other minor issues -

1. Regex Patterns Duplicated Between inputValidation.ts and InvalidNameValidator

Files:

• src/utils/validation/inputValidation.ts
• src/utils/validation/validators/InvalidNameValidator.ts

// inputValidation.ts
const NODE_NAME_PATTERN = /^[a-zA-Z][a-zA-Z0-9_\s]*$/;
const DATASET_NAME_PATTERN = /^[a-z][a-z0-9_]*$/;

// InvalidNameValidator.ts (inline, not imported)
if (!/^[a-zA-Z][a-zA-Z0-9_\s]*$/.test(trimmed)) {  // node
if (!/^[a-z][a-z0-9_]*$/.test(trimmed)) {            // dataset

Two copies of the same patterns. A change to the node naming rules (e.g., allowing hyphens) must be made in two places. The patterns should be exported from a single source (likely inputValidation.ts since it already has named constants for them) and imported by InvalidNameValidator.

---

2. EmptyNameValidator Uses Hardcoded Sentinel Strings

File: src/utils/validation/validators/EmptyNameValidator.ts

if (!name || name === 'Unnamed Node') {    // hardcoded sentinel
if (!name || name === 'Unnamed Dataset') {  // hardcoded sentinel

If the UI's default placeholder name changes from 'Unnamed Node' to 'New Node' or 'Untitled', this check silently stops working — new components pass as named when they shouldn't. These strings should be referenced from the same constant used by the UI to set default names (wherever that constant lives).

[rashidakanchwala]

      // Find specific error types for better user feedback
      const cycleError = validationResult.errors.find(e => e.message.includes('Circular dependency'));
      const duplicateError = validationResult.errors.find(e => e.message.includes('Duplicate'));
      const invalidNameError = validationResult.errors.find(e => e.message.includes('Invalid'));
      const emptyNameError = validationResult.errors.find(e => e.message.includes('no name'));

Use enums for validation types instead of message-string matching.

Current logic checks e.message.includes(...) (e.g. Duplicate/Invalid), which is brittle to wording changes. Add ValidationCode enum (e.g. DUPLICATE_NAME, INVALID_NAME, etc.) to ValidationError, emit it from validators, and switch UI checks to e.code === ValidationCode.DUPLICATE_NAME.

[jitu5]

      // Find specific error types for better user feedback
      const cycleError = validationResult.errors.find(e => e.message.includes('Circular dependency'));
      const duplicateError = validationResult.errors.find(e => e.message.includes('Duplicate'));
      const invalidNameError = validationResult.errors.find(e => e.message.includes('Invalid'));
      const emptyNameError = validationResult.errors.find(e => e.message.includes('no name'));

Use enums for validation types instead of message-string matching.

Current logic checks e.message.includes(...) (e.g. Duplicate/Invalid), which is brittle to wording changes. Add ValidationCode enum (e.g. DUPLICATE_NAME, INVALID_NAME, etc.) to ValidationError, emit it from validators, and switch UI checks to e.code === ValidationCode.DUPLICATE_NAME.

@rashidakanchwala Fixed here d3780b7