ColinEberhardt / json-transforms

A recursive, pattern-matching, approach to transforming JSON structures.
MIT License
140 stars 6 forks source link

Problem transforming a top level array of objects; object filtered out by predicate leaves "undefined" as its mark #12

Open kazurayam opened 4 months ago

kazurayam commented 4 months ago

I forked this repository, and createded a jasmine test: test/topLevelArrayCaseSpec.js

const transformer = require('./../build/json-transforms');
const identity = transformer.identity;
const pathRule = transformer.pathRule;
const transform = transformer.transform;

describe('Case of top level array of automobile objects -', () => {
  it('find Honda cars out of top level array', () => {
    const automobiles = [
      { maker: 'Nissan', model: 'Teana', year: 2011 },
      { maker: 'Honda', model: 'Jazz', year: 2010 },
      { maker: 'Honda', model: 'Civic', year: 2007 },
      { maker: 'Toyota', model: 'Yaris', year: 2008 },
      { maker: 'Honda', model: 'Accord', year: 2011 }
    ];
    const rules = [pathRule('.{.maker === "Honda"}', (d) => d.match)];
    const transformed = transform(automobiles, rules);
    expect(transformed).toEqual([
      { maker: 'Honda', model: 'Jazz', year: 2010 },
      { maker: 'Honda', model: 'Civic', year: 2007 },
      { maker: 'Honda', model: 'Accord', year: 2011 }
    ]);

    console.log('---------- topLevelArrayCaseSepec.js ----------');
    console.log(transformed);
  });
});

When I ran this test, I saw the following result in the console.

$ npm test

> json-transforms@1.1.0 test
> npm run lint && npm run bundle && jasmine JASMINE_CONFIG_PATH=test/support/jasmine.json

> json-transforms@1.1.0 lint
> eslint src/**/*.js

> json-transforms@1.1.0 bundle
> rollup -c

Treating 'jspath' as external dependency
Started
...............---------- topLevelArrayCaseSepec.js ----------
[
  undefined,
  { maker: 'Honda', model: 'Jazz', year: 2010 },
  { maker: 'Honda', model: 'Civic', year: 2007 },
  undefined,
  { maker: 'Honda', model: 'Accord', year: 2011 }
]
F...

Failures:
1) Case of top level array of automobile objects - find Honda cars out of top level array
  Message:
    Expected $.length = 5 to equal 3.
    Expected $[0] = undefined to equal Object({ maker: 'Honda', model: 'Jazz', year: 2010 }).
    Expected $[1].model = 'Jazz' to equal 'Civic'.
    Expected $[1].year = 2010 to equal 2007.
    Expected $[2].model = 'Civic' to equal 'Accord'.
    Expected $[2].year = 2007 to equal 2011.
    Expected $[4] = Object({ maker: 'Honda', model: 'Accord', year: 2011 }) to equal undefined.
  Stack:
    Error: Expected $.length = 5 to equal 3.
    Expected $[0] = undefined to equal Object({ maker: 'Honda', model: 'Jazz', year: 2010 }).
    Expected $[1].model = 'Jazz' to equal 'Civic'.
    Expected $[1].year = 2010 to equal 2007.
    Expected $[2].model = 'Civic' to equal 'Accord'.
    Expected $[2].year = 2007 to equal 2011.
    Expected $[4] = Object({ maker: 'Honda', model: 'Accord', year: 2011 }) to equal undefined.
        at UserContext.<anonymous> (/Users/kazuakiurayama/github/json-transforms/test/topLevelArrayCaseSpec.js:17:25)

19 specs, 1 failure
Finished in 0.054 seconds

Let me explain this case again in another wording.

I have a JSON as input:

const automobiles = [
      { maker: 'Nissan', model: 'Teana', year: 2011 },
      { maker: 'Honda', model: 'Jazz', year: 2010 },
      { maker: 'Honda', model: 'Civic', year: 2007 },
      { maker: 'Toyota', model: 'Yaris', year: 2008 },
      { maker: 'Honda', model: 'Accord', year: 2011 }
    ];

Please note that the automobiles has, at the top level, an array of objects. This is the point that causes json-transforms@1.1.2 behave wrongly.

I wrote a pathRule as this:

const rules = [pathRule('.{.maker === "Honda"}', (d) => d.match)];

I intended to select the Honda cars while stripping other makers off.

I expected to get the following result:

    expect(transformed).toEqual([
      { maker: 'Honda', model: 'Jazz', year: 2010 },
      { maker: 'Honda', model: 'Civic', year: 2007 },
      { maker: 'Honda', model: 'Accord', year: 2011 }
    ]);

But in fact, json-transforms@1.1.2 generated the following result:

[
  undefined
  { maker: 'Honda', model: 'Jazz', year: 2010 },
  { maker: 'Honda', model: 'Civic', year: 2007 },
  undefined,
  { maker: 'Honda', model: 'Accord', year: 2011 }
]

I think that undefined should NOT be present in the result array.

kazurayam commented 4 months ago

I read the source of json-transforms@1.1.2. I have found out a fix for this issue.

I changed the src/transform.js.

origin: https://github.com/ColinEberhardt/json-transforms/blob/v1.1.2/src/transform.js

My proposal:

const transform = (json, rules) => {

  const runner = match => {
    for (let i = 0; i < rules.length; i++) {
      const rule = rules[i];
      const res = rule(match, adaptedRunner);
      if (res !== null) {
        return res;
      }
    }
    // at this point, not yet returned.
    // the runner function will return "undefined".
  };

  const adaptedRunner = ast => {
    if (Array.isArray(ast)) {
      let result = ast.map(r => runner(r));
      // a call to runner(r) may return "undefined".
      // "undefined" should not appear in the result array.
      // so let's filter it out.
      result = result.filter((e) => {
        return e !== undefined;
      });
      return result;
    } else {
      return runner(ast);
    }
  };

  return adaptedRunner(json);
};

export default transform;

I inserted some lines of comments where I explained my idea: what caused "undefined" to be present in the resulting array, how to remove "undefined" away.

With this change to transform.js applied, the test/topLevelArrayCaseSpec.js passed. The problem has been fixed.

I am going to create a Pull Request shortly.