code-review

Code Review

Use this skill when reviewing SynapseML changes.

Steps

1. Inspect diff: git --no-pager diff --stat && git --no-pager diff
2. Run Scala style: sbt scalastyle test:scalastyle
3. Run Python format check: black --check --extend-exclude 'docs/' .
4. Run targeted tests for touched code.
5. Apply the checklists below to every changed file.
6. Report only concrete issues with file paths and fixes.

Security Checklist

Apply when changes touch serialization, I/O, network, or authentication code.

Deserialization (CWE-502)

• No raw ObjectInputStream.readObject(): use SafeObjectInputStream with an allowlist
• resolveClass allowlist validates array component types. Never allowlist the [ prefix directly; array handling must extract and validate the component class name
• resolveProxyClass is overridden to block or validate dynamic proxy interfaces
• Allowlist uses package-prefix matching, not blocklisting
• Allowlist presets contain no catch-all entries that bypass the filtering logic

Input Validation

• File paths, URLs, and user-supplied strings are validated before use
• No unsanitized string interpolation into SQL, shell commands, or config

Resource Management

• Streams, connections, and closeable resources use using() or try-finally
• Cleanup runs even when assertions or exceptions are thrown (especially in tests)

Secrets & Credentials

• No hardcoded secrets, tokens, or passwords in source or test code
• Credentials loaded from environment variables or secure config only

API Compatibility Checklist

Apply when changes modify public classes, traits, or companion objects.

Binary Compatibility (JVM)

• No method signature changes on existing public methods (default parameters generate synthetic bridges; use explicit overloads instead)
• No removed or renamed public classes, traits, or objects
• Companion object extends DefaultParamsReadable[T] preserved if it existed

Source Compatibility

• New parameters have defaults so existing callers compile unchanged
• No narrowed return types or widened parameter types on public methods
• Import changes don't break wildcard imports in downstream code

Scala Checklist

• License header present (enforced by scalastyle)
• Wrappable trait mixed in if the class needs a Python wrapper
• SynapseMLLogging trait mixed in; logClass() called in constructor
• No wildcard imports where explicit imports suffice (java.io._ → named imports)
• No RDD API usage. Use DataFrame/Dataset only
• Lines ≤ 120 chars, files ≤ 800 lines

Python Checklist

• License header present
• Formatted with black==22.3.0
• No edits to files under target/ (auto-generated)
• Hand-written overrides in src/main/python/ extend the generated _ClassName

Test Checklist

• New functionality has corresponding tests
• Tests use using() for resource cleanup (no bare .close() after assertions)
• Negative tests verify rejection/error cases, not just happy paths
• No test-only dependencies leaked into main scope

Python isolation and async cleanup

Apply when changing Spark test setup, optional SDK support, or async batching.

• Imports that can create a Spark session happen after the required Spark fixture initializes, unless startup itself is under test. Reproduce cold-start failures in a fresh process and verify actual JVM class/JAR loading; a previously initialized session can hide the wrong setup.
• No-SDK tests exercise the public import path in a fresh process on the branch's Python. Import blockers implement find_spec and raise ImportError for blocked dependencies; returning None permits another finder to load them. Unrelated imports still work.
• Async tasks and loop-bound clients are created and used on their owning loop. A loop running on another thread receives thread-safe submissions, not calls that drive it from the test/caller thread.
• Fail-fast batches cancel and await pending siblings before returning the original failure. Tests assert no orphaned requests remain, including progress-enabled paths, and clean up even when the assertion fails.
• Supported idle, same-thread nested, and foreign-thread loop paths are covered. Optional nested-loop dependencies have explicit missing-dependency behavior without breaking paths that do not require them.