fix(graphql): column with complex object could throw null pointer exception

[Harsgalt86]

As per discussion

Unit test:

it('should return a query string including a direct reference to a complex object', () => {
  const expectation = `firstName, lastName, billing{address{street, zip}}`;
  const columns = ['firstName', 'lastName', 'billing', 'billing.address.street', 'billing.address.zip'];

  const query = service.buildFilterQuery(columns);

  expect(removeSpaces(query)).toBe(removeSpaces(expectation));
});

Fix:

const set = (o: any = {}, a: any) => {
  const k = a.shift();
  o[k] = a.length ? set(o[k] ?? undefined, a) : null;
  return o;
};

By passing undefined it will use the default {} as per the parameter signature.
Currently it is pasing null which will not use TypeScript's default value functionality.
In TypeScript, a property only makes use of the default value if the property is undefined, not null

set(null, a) => o will have value null
set(undefined, a) => o will have value {}

Note: I did try setting the line to be o[k] = a.length ? set(o[k], a) : undefined; but it broke other tests (most likely because of strict equality checks?)

[ghiscoding]

it looks like there's a Linting problem in the new unit test, could you please fix it

/home/runner/work/slickgrid-universal/slickgrid-universal/packages/graphql/src/services/graphqlQueryBuilder.ts
Error: 8:42 error Trailing spaces not allowed no-trailing-spaces

[ghiscoding]

so the only difference is that you added billing and this was previously throwing an error, is that it?

[Harsgalt86]

Yeah, that's right

[ghiscoding]

from your description, you say doing this would force the use of the default {} but in that case why not provide it directly here instead? Wouldn't it be better to simply use set(o[k] ?? {}, a)

[Harsgalt86]

Sure, I can do that if you prefer.

However, the benefit of passing undefined, is that if you decide to change the default value to something else in the future, you won't have to update the places it's passed in.
eg:
Changing the signature to
const set = (o: any = { a: true }, a: any)
Means updating
set(o[k] ?? { a: true }, a) // to match the new default value
While, doesn't need to change
set(o[k] ?? undefined, a) // o will end up having the update default value as specified

[ghiscoding]

I don't see why it would ever change and I also think passing {} is much more obvious because I would not have imagined that it would use the default until you said it would, it's less clear with undefined as opposed to {} which to me seems clearer what the change and intention is

[Harsgalt86]

it looks like there's a Linting problem in the new unit test

/home/runner/work/slickgrid-universal/slickgrid-universal/packages/graphql/src/services/graphqlQueryBuilder.ts
Error: 8:42 error Trailing spaces not allowed no-trailing-spaces

This is really weird?

1. Doesn't happen locally running pnpm run bundle
2. I haven't changed that file
3. It doesn't look like there's a trailing space at all?
   

[ghiscoding]

it looks like there's a Linting problem in the new unit test

/home/runner/work/slickgrid-universal/slickgrid-universal/packages/graphql/src/services/graphqlQueryBuilder.ts
Error: 8:42 error Trailing spaces not allowed no-trailing-spaces

This is really weird?

1. Doesn't happen locally running pnpm run bundle
2. I haven't changed that file
3. It doesn't look like there's a trailing space at all?
   

ah sorry that was actually caused by me when I edited the file through GitHub a few minutes ago, oops sorry about that. But since you pulled my code, it fails your PR, could you simply fix it anyway in your PR, I added an extra space by mistake as shown below on line 8 of graphqlQueryBuilder.ts file. it's a bit weird that ESLint complains in a comment, but that's where the error is.

https://github.com/ghiscoding/slickgrid-universal/blob/8d6c46fbafb32959f86e8abeec60f959c6a48878/packages/graphql/src/services/graphqlQueryBuilder.ts#L8C2-L9

[codecov]

Codecov Report

Merging #1130 (42ce5a9) into master (37d0c3c) will not change coverage.
Report is 1 commits behind head on master.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master     #1130   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          245       245           
  Lines        16886     16886           
  Branches      6062      6062           
=========================================
  Hits         16886     16886           

Files	Coverage Δ
packages/graphql/src/services/graphql.service.ts	100.00% <100.00%> (ø)
...ckages/graphql/src/services/graphqlQueryBuilder.ts	100.00% <ø> (ø)

[ghiscoding]

@Harsgalt86 shipped in v3.3.2 and all other libs.
Thanks again for your contribution ⭐