chore: removing dependency on deprecated schemaValidator @W-20495414@ #1362

jfeingold35 · 2025-12-16T19:23:32Z

What does this PR do?

This PR replaces the plugin-data usage of the now-deprecated schemaValidator class from sfdx-core with a zod-based validation of the schema. This is apparently the last remaining reference to schemaValidator, clearing the way for us to eventually remove it.

What issues does this PR fix or reference?

@W-20495414@

mshanemc · 2025-12-16T20:58:36Z

src/api/data/tree/importPlan.ts

    ) as SObjectTreeInput;

 export const validatePlanContents =
  (logger: Logger) =>


DataPlanPartFilesOnly shouldn't exist, it should be derived/inferred from the zod type.
and that schema should only have the real fields (not saveRefs/resolveRefs)

pro-tip: definitely read and understand the zod docs. by default, zod.object will strip unknown props. .strictObject will throw if any are present, and .looseObject will let them stay on the result.

so maybe warn about extra properties based on the planContents and then .object will strip them for you.

[FWIW, I'd also suggest using the actual CLI (oclif) warnings to help people migrate, rather than Logger which nobody will see]

Okay, I didn't realize that part of the plan here was to remove the DataPlanPartFIlesOnly. I was trying to preserve the existing type definitions and behavior, changing only the implementation of the validation mechanism. If it's actually desirable to change those things, then I will do so. I will ask if I have any follow-up questions.

mshanemc · 2025-12-16T21:07:14Z

src/api/data/tree/importPlan.ts

+    if (!hasOnlySimpleFiles(output)) {
+      throw messages.createError('error.NonStringFiles');
    }
+    return Promise.resolve(planContents as DataPlanPartFilesOnly[]);


I will almost never approve an assertion in /src (I understand there's been one here for a really long time)

If we don't want to allow the DataPlanPart side of the TS union then let's block it, or manipulate it into the string[] that we expect.

The compiler here is trying to tell you that the types don't match the result type (zod definitely makes that clearer) and as is sweeping that under the rug.

Or let me back up...the output of zod.parse/safeParse is a validated, typed object (why the TS world loves it so much). And you can use .infer to get that from the schema like https://zod.dev/basics?id=inferring-types

I'd suggest looking through the consuming code to see whether it handles both scenarios (string[] vs. DataImportFile) and

if yes, account for both types in this fn's return type (simplified if you return the .infer)

if not, then we massage it here to return the expected type.

we should also be able to get rid of this assertion above since it's being passed unknown for proper validation now.

(await JSON.parse(fs.readFileSync(resolvedPlanPath, 'utf8'))) as DataPlanPartFilesOnly[]

jfeingold35 · 2025-12-17T18:43:00Z

src/schema/dataImportPlan.ts

+      files: z
+        .array(
+          z
+            .string('The `files` property of the plan objects must contain only strings')


Instead of having zod accept an object here and then having us throw an error later if that object is actually used, decided to just have zod reject with the same message.

the goals of the change are

schemaValidator => zod

warning user in the UI instead of the logs

use proper types instead of as

it's not meant to be a breaking change (imagine someone's script doing a data load, been working fine since 2020, etc)

If someone were using the old style did this break them?

My understanding is that it would not. The only thing that has changed is where the error is being thrown. Even the text of the error remains the same.

yeah, I think you're right. I'm good with this functionally, then; everything else is "how to code" stuff.

jfeingold35 · 2025-12-17T18:44:43Z

src/api/data/tree/importTypes.ts

  referenceId: string;
  id: string;
 };
-export type ImportResults = {


This type was never being used anywhere.

cool. definitely get to know npx knip (we've included that in our cursor instructions within the extensions to avoid that kind of thing)

jfeingold35 · 2025-12-17T18:45:46Z

src/api/data/tree/importPlan.ts

+import { Logger, Connection, Messages } from '@salesforce/core';
 import type { GenericObject, SObjectTreeInput } from '../../../types.js';
+import { DataImportPlanArraySchema, DataImportPlanArray } from '../../../schema/dataImportPlan.js';
 import type { DataPlanPartFilesOnly, ImportResult } from './importTypes.js';


Next step is to fully remove the DataPlanPartFIlesOnly type, since now it's only used to define a different type.

jfeingold35 · 2025-12-17T19:20:39Z

src/api/data/tree/importPlan.ts


 // the "new" type for these.  We're ignoring saveRefs/resolveRefs
-export type EnrichedPlanPart = Omit<DataPlanPartFilesOnly, 'saveRefs' | 'resolveRefs'> & {
+export type EnrichedPlanPart = Partial<DataPlanPart> & {


The properties coming from Omit<DataPlanPartFilesOnly, 'saveRefs' | 'resolveRefs'> are all also hard-defined in this type here, except for this Partial, which I included as well.
Now that type serves no purpose and could be deleted.

jfeingold35 · 2025-12-17T19:21:25Z

src/api/data/tree/importPlan.ts

+export const importFromPlan = async (
+  conn: Connection,
+  planFilePath: string,
+  display: Display


Adding a new Display parameter let me show the warning in the console instead of logging it to some never-looked-at file.

pass in oclif.ux?
ex: https://github.com/salesforcecli/plugin-templates/blob/ab0df387b1de4189880edc43815a4d68da2bbeac/src/utils/templateCommand.ts#L19

another nice thing you can do is user Lifecycle from sfdx-core with its warn feature. All the CLI plugins automatically send those to the output.

and one more option (the most test-friendly, and I think the best option) would be return an object of {result: ImportResult, warnings: string[]} so that

it's a pure function, no mocks or stubs needed to test it

the command, which has warn already, can do "ux things" and helpers like this don't need to think about ux, only data.

jfeingold35 · 2025-12-17T19:21:45Z

src/api/data/tree/importPlan.ts

+
+  for (const parsedPlan of parsedPlans) {
+    if (parsedPlan.saveRefs !== undefined || parsedPlan.resolveRefs !== undefined) {
+      display.displayWarning(


Using the display instead of the logger.

jfeingold35 · 2025-12-17T19:23:09Z

src/ux/Display.ts

+ * (for example, via {@code console.log()}).
+ * The interface/implementation pattern allows for extremely easy mocking of dependencies in tests.
+ */
+export interface Display {


This paradigm is something @stephen-carter-at-sf developed for the Code Analyzer Team. It's easily mockable, which makes it really easy to test. If we ever (for some reason) need to add new methods, we can do that by putting them in the interface, and having the UxDisplay just invoke the corresponding method on the Displayable.

Stephen probably didn't know about
https://github.com/salesforcecli/sf-plugins-core/blob/main/src/ux/ux.ts
and https://github.com/salesforcecli/sf-plugins-core/blob/main/src/stubUx.ts#L42.

I'd like for this not to exist. not only to use existing patterns with plugins, but to encourage other teams and AIs to do the same

meta feedback: when you think of somethign like this, ask on slack because often something exists.

I'll look over this tomorrow to see how I would use it in place of the Display system.

it's good to know about, would still recommend returning {results, warnings} from the fn

jfeingold35 · 2025-12-17T19:24:26Z

src/ux/Display.ts

+}
+
+export interface Displayable {
+  warn(message: string): void;


Using the warn name allows us to have the tree command implement Displayable and pass itself into the constructor of the Display that it creates and passes through into validation.

mshanemc · 2025-12-17T23:06:46Z

src/api/data/tree/importPlan.ts

      Object.entries(record).map(([k, v]) => [k, v === `@${ref.refId}` ? ref.id : v])
    ) as SObjectTreeInput;

-export const validatePlanContents =


meta: don't break the rule and then add the comment to allow you to break the rule unless you've got a really compelling reason (and preferably explain your reason!)

here: you can export it for test only, it won't cause it to be publicly available unless you export it from index.js or some other package.json#exports file (I'm trying to guess why you'd want to do that)

Fair enough. I'm used to naming things that are exported solely for tests _blah instead of blah, because it makes it visually obvious to anyone reading the code that this method should be treated as private. The underscore signals 'oh, I shouldn't import this to whatever other code I'm writing'.
Part of my hesitance to futz with the code heavily the first time around was because so many things were exported. It wasn't immediatley obvious what was test-only and what wasn't.

Sure, we can talk about why that distinction is important.

mshanemc · 2025-12-18T16:12:19Z

QA notes: (using ./bin/dev.js and the test/data-project stuff from this repo
✅ warning about unused props appears, looks good in yellow
✅ no warning without unused props
✅ error happens when you have bad schema
⚠️ it's kinda not clear what I did wrong (omit the files prop on one of the plan objects

Data plan file /Users/shane.mclaughlin/eng/salesforcecli/plugin-data/test/test-files/data-project/data/accounts-contacts-plan-object.json did not validate against the schema. Errors: Invalid input: expected array, received undefined.

zod error looks like this

error: ZodError: [
    {
      "expected": "array",
      "code": "invalid_type",
      "path": [
        0,
        "files"
      ],
      "message": "Invalid input: expected array, received undefined"
    }
  ]

so that path prop is important.

❌ when the schema validation fails, there's a reference to a command that no longer exists in the "try this" (was not caused by this PR)

mshanemc reviewed Dec 16, 2025

View reviewed changes

chore: stop using deprecated schemaValidator @W-20495414@

aea89d1

jfeingold35 force-pushed the jf/W-20495414 branch from 5e41d60 to aea89d1 Compare December 17, 2025 16:49

jfeingold35 commented Dec 17, 2025

View reviewed changes

jfeingold35 added 2 commits December 17, 2025 13:00

chore: removed DataPlanPartFileOnly type @W-20495414@

b5985ca

chore: warning is displayed instead of logged @W-20495414@

b88d798

jfeingold35 commented Dec 17, 2025

View reviewed changes

mshanemc reviewed Dec 17, 2025

View reviewed changes

jfeingold35 added 2 commits December 18, 2025 08:36

chore: integrate feedback from code review @W-20495414@

c458367

chore: removed leading underscore @W-20495414@

981cd11

mshanemc approved these changes Dec 18, 2025

View reviewed changes

jfeingold35 merged commit 815eb41 into main Dec 18, 2025
64 of 65 checks passed

jfeingold35 deleted the jf/W-20495414 branch December 18, 2025 15:29

mshanemc mentioned this pull request Dec 18, 2025

fix: improve error message for schema problems W-20495414 #1363

Merged

chore: removing dependency on deprecated schemaValidator @W-20495414@ #1362

chore: removing dependency on deprecated schemaValidator @W-20495414@ #1362

Uh oh!

Conversation

jfeingold35 commented Dec 16, 2025

What does this PR do?

What issues does this PR fix or reference?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mshanemc Dec 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mshanemc Dec 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mshanemc commented Dec 18, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mshanemc Dec 16, 2025 •

edited

Loading

mshanemc Dec 17, 2025 •

edited

Loading