fix: revert breaking change in results creation

[wellwelwel]

The previous solution (#2574) aimed to create a clean object, but it caused a major change.

Instead, I just grouped the object's private properties:

const privateObjectProps = new Set([
  '__defineGetter__',
  '__defineSetter__',
  '__lookupGetter__',
  '__lookupSetter__',
  '__proto__',
]);

Then, it will perform a simple validation to ensure that the fields[i].name is safe:

if (helpers.privateObjectProps.has(fields[i].name)) {
  throw new Error(
    `The field name (${fieldName}) can't be the same as an object's private property.`,
  );
}

By this way, it's possible to:

• use the native methods available from an object
• deep compare results with an expected variable
• test the vulnerability without use a query for that
• If an user, for some reason, wants to create or customize a prototype after the query has been finished, they can

Basically, as it should be for a patch/minor bump 🙋🏻‍♂️

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.32%. Comparing base (6dccf55) to head (a866100).
Report is 2 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #2591   +/-   ##
=======================================
  Coverage   90.32%   90.32%           
=======================================
  Files          71       71           
  Lines       15717    15725    +8     
  Branches     1334     1339    +5     
=======================================
+ Hits        14196    14204    +8     
  Misses       1521     1521           

Flag	Coverage Δ
compression-0	90.32% <100.00%> (+<0.01%)	⬆️
compression-1	90.32% <100.00%> (+<0.01%)	⬆️
tls-0	89.85% <100.00%> (+<0.01%)	⬆️
tls-1	90.14% <100.00%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[sidorares]

Let's run some benchmarks before merging

[wellwelwel]

@sidorares, I rewrote the logic and description of this PR.

Could you check again? 🙋🏻‍♂️

[wellwelwel]

Using Set and has instead of Array and includes, due to:

Basic benchmark:

const arrayIncludes = (field) => {
  console.time('Array.includes');

  const privateObjectProps = [
    '__defineGetter__',
    '__defineSetter__',
    '__lookupGetter__',
    '__lookupSetter__',
    '__proto__',
  ];

  for (let i = 0; i < 1000000; i++) privateObjectProps.includes(field);

  console.timeEnd('Array.includes');
};

const setHas = (field) => {
  console.time('Set.has');

  const privateObjectProps = new Set([
    '__defineGetter__',
    '__defineSetter__',
    '__lookupGetter__',
    '__lookupSetter__',
    '__proto__',
  ]);

  for (let i = 0; i < 1000000; i++) privateObjectProps.has(field);

  console.timeEnd('Set.has');
};

for (let i = 0; i < 5; i++) arrayIncludes('field');
for (let i = 0; i < 5; i++) setHas('field');

---

Results:

Array.includes: 1.869ms
Array.includes: 0.621ms
Array.includes: 0.644ms
Array.includes: 0.658ms
Array.includes: 0.677ms
Set.has: 1.275ms
Set.has: 0.572ms
Set.has: 0.599ms
Set.has: 0.619ms
Set.has: 0.553ms

---

Compatibility:

Set: Node.js 0.12.18.

[wellwelwel]

Bringing a comment here: 74abf9e#r140992502 🤝

[wellwelwel]

As suggested by @sidorares:

Instead of checking the field name manually, we have some alternatives:

• Refactor the results object to a mapped object
• Keep it as Object.create(null) by adding a config option to use a standard object for results (not sure about bump a major version for this)

---

Although the issue #2585 doesn't have that many interactions, I think it's important to consider the users who don't participate directly 🙋🏻‍♂️

Also, if we consider the current solution, it's possible to return an empty object instead of throwing an error, as in mysqljs/mysql.

[wellwelwel]

Merging this.

For a major release, I recommend reverting this PR and include a safer approach in docs, such as:

Object.hasOwn(row, 'column');

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/hasOwnProperty#objects_created_with_object.createnull

[ert78gb]

thank you so much for the solution