Merge pull request #2 from apify/claude/safepushdata-placeholder-values-m9ncfy

change: simplify placeholder strategy to only use non-controversial empty values

Author: metalwarrior665

README.md

@@ -47,25 +47,30 @@ every AJV error per item:
 ### Placeholder defaults
 
 When a constraint fires on a path we placeholder'd ourselves, the wrapper
-picks a value that should satisfy it:
-
-| AJV keyword                                             | Placeholder value               |
-| ------------------------------------------------------- | ------------------------------- |
-| `type: string`                                          | `''`                            |
-| `type: integer` / `number`                              | `0`                             |
-| `type: boolean`                                         | `false`                         |
-| `type: array`                                           | `[]`                            |
-| `type: object`                                          | `{}`                            |
-| `type: null`                                            | `null`                          |
-| `minLength: N` / `maxLength`                            | `'_'.repeat(N)` / `''`          |
-| `minimum: N` / `maximum`                                | `N`                             |
-| `exclusiveMinimum: N` / `exclusiveMaximum`              | `N + 1` / `N - 1`               |
-| `enum`                                                  | First allowed value             |
-| `format: email` / `uri` / `date` / `date-time` / `uuid` | a static valid example for each |
-| Anything else (`pattern`, custom formats…)              | Item is dropped.                |
+picks a value that should satisfy it. We deliberately only fill in the four
+**empty** values below — they're unambiguously empty and can't be mistaken
+for real data:
+
+| AJV keyword    | Placeholder value |
+| -------------- | ----------------- |
+| `type: string` | `''`              |
+| `type: array`  | `[]`              |
+| `type: object` | `{}`              |
+| `type: null`   | `null`            |
+| Anything else  | Item is dropped.  |
+
+When a field allows **multiple types** (e.g. `['string', 'null']`), the
+wrapper always picks `null` — it's the cleanest placeholder because it
+commits to no concrete value at all.
+
+Everything else (`enum`, `format`, `minLength`, numeric bounds, `type:
+integer` / `number` / `boolean`, …) is **not** placeholdered: a made-up
+email, a first-enum-value, or a fabricated number would silently poison the
+customer's dataset with plausible-looking junk, so the item is dropped
+instead.
 
 The retry loop chases one layer of errors per round
-(`required` → `type` → `minLength` → push) until either the push succeeds
+(`required` → `type` → push) until either the push succeeds
 or `maxAttempts` (default 5) is hit.
 
 ## Options

src/safePushData.ts

@@ -1,6 +1,13 @@
 // safePushData: parse the Apify dataset schema-validation error, repair the
 // offending items (strip bad fields, placeholder missing required ones), and
 // retry the push.
+//
+// NOTE: instead of recursively healing the data one error-round at a time, we
+// could parse the Actor's `dataset_schema.json` up front and fix every item in
+// a single pass (we'd know each field's expected type / constraints without
+// waiting for the API to report them). That would avoid the multi-round retry
+// loop, but requires heavier code (locating + loading the schema, resolving
+// $refs, walking the schema tree). Something to consider in the future.
 
 const SCHEMA_ERROR_TYPE = 'schema-validation-error';
 
@@ -234,77 +241,48 @@ function cleanItemFields<T>(item: T, validationErrors: ValidationError[], placeh
 }
 
 // Pick a value that will satisfy `err.keyword` on a placeholder field.
-// Returns ok:false when we don't have a sensible default (e.g. `pattern`,
-// custom formats); the caller drops the item in that case.
+// Returns ok:false when we don't have a sensible default; the caller drops
+// the item in that case.
+//
+// We deliberately only placeholder the four "empty" values — `''`, `[]`, `{}`,
+// and `null`. These are unambiguously empty and can't be mistaken for real
+// data. We do NOT fabricate values for `enum`, `format`, `minLength`, numeric
+// bounds, etc.: a made-up email, a first-enum-value, or a `'_'.repeat(N)`
+// string all silently poison the customer's dataset with plausible-looking
+// junk. Better to drop the item than to lie about its contents. As a result
+// the only keyword we handle is `type` (only for those four target types) —
+// everything else falls through to ok:false and the item is dropped.
 function placeholderFor(err: ValidationError): { ok: true; value: unknown } | { ok: false } {
     const params = err.params ?? {};
-    switch (err.keyword) {
-        case 'type': {
-            // params.type is the expected type as a string, or array of strings.
-            const t = Array.isArray(params.type) ? params.type[0] : params.type;
-            switch (t) {
-                case 'string':
-                    return { ok: true, value: '' };
-                case 'integer':
-                case 'number':
-                    return { ok: true, value: 0 };
-                case 'boolean':
-                    return { ok: true, value: false };
-                case 'array':
-                    return { ok: true, value: [] };
-                case 'object':
-                    return { ok: true, value: {} };
-                case 'null':
-                    return { ok: true, value: null };
-                default:
-                    break;
-            }
-            return { ok: false };
-        }
-        case 'minLength': {
-            const limit = Number(params.limit) || 1;
-            return { ok: true, value: '_'.repeat(limit) };
-        }
-        case 'maxLength':
-            return { ok: true, value: '' };
-        case 'minimum':
-        case 'exclusiveMinimum': {
-            const limit = Number(params.limit);
-            if (!Number.isFinite(limit)) return { ok: false };
-            return { ok: true, value: err.keyword === 'exclusiveMinimum' ? limit + 1 : limit };
-        }
-        case 'maximum':
-        case 'exclusiveMaximum': {
-            const limit = Number(params.limit);
-            if (!Number.isFinite(limit)) return { ok: false };
-            return { ok: true, value: err.keyword === 'exclusiveMaximum' ? limit - 1 : limit };
-        }
-        case 'enum': {
-            const allowed = params.allowedValues;
-            if (Array.isArray(allowed) && allowed.length > 0) return { ok: true, value: allowed[0] };
-            return { ok: false };
-        }
-        case 'format': {
-            switch (params.format) {
-                case 'email':
-                    return { ok: true, value: 'placeholder@example.com' };
-                case 'uri':
-                case 'uri-reference':
-                case 'url':
-                    return { ok: true, value: 'about:blank' };
-                case 'date':
-                    return { ok: true, value: '1970-01-01' };
-                case 'date-time':
-                    return { ok: true, value: '1970-01-01T00:00:00Z' };
-                case 'uuid':
-                    return { ok: true, value: '00000000-0000-0000-0000-000000000000' };
-                default:
-                    break;
-            }
-            return { ok: false };
+    if (err.keyword !== 'type') return { ok: false };
+
+    // params.type is the expected type as a string, or an array of strings
+    // when the field allows multiple types (e.g. `['string', 'null']`).
+    const types = Array.isArray(params.type) ? params.type : [params.type];
+
+    // Union type that permits null: prefer null. It's the cleanest possible
+    // placeholder — it commits to no concrete value at all — so whenever the
+    // schema allows it, that's what we use.
+    if (types.length > 1 && types.includes('null')) {
+        return { ok: true, value: null };
+    }
+
+    // Otherwise take the first allowed type we have an "empty" default for.
+    // integer / number / boolean are intentionally absent: 0 / false read as
+    // real data, so a field of only those types is dropped instead.
+    for (const t of types) {
+        switch (t) {
+            case 'null':
+                return { ok: true, value: null };
+            case 'string':
+                return { ok: true, value: '' };
+            case 'array':
+                return { ok: true, value: [] };
+            case 'object':
+                return { ok: true, value: {} };
+            default:
+                break;
         }
-        default:
-            break;
     }
     return { ok: false };
 }

test/safePushData.test.ts

@@ -125,8 +125,11 @@ test('placeholders a missing required field, then satisfies the type', async ()
     assert.deepEqual(calls[calls.length - 1][0], { age: 30, name: '' });
 });
 
-test('chases required -> type -> minLength on the same placeholder field', async () => {
+test('drops item when a placeholder field carries a minLength it cannot satisfy', async () => {
     // Schema: { required: ['name'], properties: { name: { type: 'string', minLength: 3 } } }
+    // We placeholder name to '' (type: string), but we no longer fabricate a
+    // `'_'.repeat(N)` string for minLength — a made-up string is customer-data
+    // poison — so the item is dropped instead.
     const validate = (item: Item): ValidationError[] | null => {
         const errors: ValidationError[] = [];
         if (item?.name === undefined) {
@@ -158,14 +161,17 @@ test('chases required -> type -> minLength on the same placeholder field', async
         }
         return null;
     };
-    const { pushFn, calls } = makeMockPush(validate);
+    const { pushFn } = makeMockPush(validate);
     const res = await safePushData(pushFn, { age: 30 }, { maxAttempts: 10 });
-    assert.equal(res.pushed, 1);
-    assert.equal(res.attempts, 4); // required, type, minLength, success
-    assert.equal((calls[calls.length - 1][0].name as string).length, 3);
+    assert.equal(res.pushed, 0);
+    assert.equal(res.dropped.length, 1);
+    assert.deepEqual(res.dropped[0].item, { age: 30 });
 });
 
-test('placeholder for enum picks the first allowed value', async () => {
+test('drops item on enum constraint instead of fabricating the first allowed value', async () => {
+    // We used to placeholder an enum field with its first allowed value; that
+    // silently injects a plausible-but-wrong value into the dataset, so we now
+    // drop the item instead.
     const validate = (item: Item): ValidationError[] | null => {
         if (item?.role === undefined) {
             return [
@@ -200,13 +206,15 @@ test('placeholder for enum picks the first allowed value', async () => {
         }
         return null;
     };
-    const { pushFn, calls } = makeMockPush(validate);
-    const res = await safePushData(pushFn, { name: 'x' });
-    assert.equal(res.pushed, 1);
-    assert.equal(calls[calls.length - 1][0].role, 'admin');
+    const { pushFn } = makeMockPush(validate);
+    const res = await safePushData(pushFn, { name: 'x' }, { maxAttempts: 10 });
+    assert.equal(res.pushed, 0);
+    assert.equal(res.dropped.length, 1);
 });
 
-test('placeholder for format=email', async () => {
+test('drops item on format=email instead of fabricating a fake address', async () => {
+    // A made-up `placeholder@example.com` is exactly the kind of junk we no
+    // longer inject; the item is dropped instead.
     const validate = (item: Item): ValidationError[] | null => {
         if (item?.email === undefined) {
             return [
@@ -240,10 +248,45 @@ test('placeholder for format=email', async () => {
         }
         return null;
     };
+    const { pushFn } = makeMockPush(validate);
+    const res = await safePushData(pushFn, { name: 'x' }, { maxAttempts: 10 });
+    assert.equal(res.pushed, 0);
+    assert.equal(res.dropped.length, 1);
+});
+
+test('required field with a union type that allows null is placeholder-filled with null', async () => {
+    // Schema: { required: ['note'], properties: { note: { type: ['string', 'null'] } } }
+    // The initial `required` placeholder sets note = null; because the field
+    // allows null, the follow-up type error reports both allowed types and we
+    // keep null (the cleanest placeholder) rather than coercing to ''.
+    const validate = (item: Item): ValidationError[] | null => {
+        if (!('note' in (item ?? {}))) {
+            return [
+                {
+                    instancePath: '',
+                    keyword: 'required',
+                    params: { missingProperty: 'note' },
+                    message: "must have required property 'note'",
+                },
+            ];
+        }
+        if (item.note !== null && typeof item.note !== 'string') {
+            return [
+                {
+                    instancePath: '/note',
+                    keyword: 'type',
+                    params: { type: ['string', 'null'] },
+                    message: 'must be string,null',
+                },
+            ];
+        }
+        return null;
+    };
     const { pushFn, calls } = makeMockPush(validate);
     const res = await safePushData(pushFn, { name: 'x' });
     assert.equal(res.pushed, 1);
-    assert.equal(calls[calls.length - 1][0].email, 'placeholder@example.com');
+    assert.equal(res.dropped.length, 0);
+    assert.deepEqual(calls[calls.length - 1][0], { name: 'x', note: null });
 });
 
 test('drops item when a placeholder constraint has no known fix (pattern)', async () => {