Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Multi wildcard support finalization #40

Merged
merged 11 commits into from
Jan 20, 2022
Merged

Multi wildcard support finalization #40

merged 11 commits into from
Jan 20, 2022

Conversation

lrecknagel
Copy link
Contributor

@lrecknagel lrecknagel commented Jan 14, 2022

First - thanks for all the efforts already put in this project and also the PR with I`am picking up!

2nd - I hope I have picked up the prior work correctly, please tell me if I take a wrong path here.
(Followed the suggested manual: https://gist.github.com/jsumners/461ef7a64545108635cc437fde112721)

I´ve worked on finishing the multi-wildcard feature #36 with the missing test-coverage.
Also added handling for a undefined / null case which I`ve found.

Following checks have been made:

  • npm run cov-ui
  • npm run test
  • npm run bench (see attached report)

fast-redact-bench-wc_finalization.txt

Please give me ping if I forgot something - hope we can bring this feature to life!

Best Lucas

target[key] = value
}
/* istanbul ignore else */
if (typeof target === 'object') {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible that target is not an object? It's not covered by tests anyway

Copy link
Contributor Author

@lrecknagel lrecknagel Jan 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think its possible, applies for the example below:

const redact = fastRedact({
  paths: ['a[*].x.d[*].i.*.i']
})
const o = {
  a: [
    { x: { d: [ { i: undefined, j: 'NR' } ] } }
  ]
}

Only applies when the given path expects a deeper nested object but is undefined somewhere.
I`am not sure if this should be a case to be considered.

const targetKeys = Object.keys(target)
for (var j = 0; j < targetKeys.length; j++) {
const tKey = targetKeys[j]
const subTarget = target[tKey]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what if subtarget is also an object?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you pls explain your point here - I dont get it right now
subTarget can be an object, and it works imho

For the example, which is also in the tests (https://github.com/lrecknagel/fast-redact/blob/multi-wildcard-support-finalization/test/index.js#L1228)

const redactOut = fastRedact({
  paths: ['a[*].x.d[*].p.*.i'],
})
const obj = {
  a: [
    { x: { d: [ { i: undefined, j: 'NR' } ] }},
  ]
}
redactOut(obj)

we get the following state for subtarget:

Bildschirmfoto 2022-01-19 um 13 48 31

lib/modifiers.js Outdated Show resolved Hide resolved
lib/modifiers.js Outdated Show resolved Hide resolved
Copy link
Owner

@davidmarkclements davidmarkclements left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

exceptional work, thank you!

couple of points, also would you mind posting benchmarks of main branch vs your branch, and also adding benchmarks that cover this new code (if it's not covered already that is)

@mcollina
Copy link
Collaborator

Can you also rebase on top of main so we get some CI running? Thanks!

Copy link
Collaborator

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm on passing CI

@lrecknagel
Copy link
Contributor Author

Thanks for the hints and comments! I´ve working on the benchmark part and will push it asap.

@lrecknagel
Copy link
Contributor Author

  • Two simple multi-wc benchmarks added.
  • See bench of main and the current one:

001_fast-redact-bench-main.txt
002_fast-redact-bench-wc_finalization.txt

Depending on getObj which gives a relatively flat object it might be interesting to add another multi-wc bench on deeper nested objects. Would be great if you could express your opinion on that shortly.

@mcollina
Copy link
Collaborator

I'm good with the benchmarks I'm seeing

@davidmarkclements
Copy link
Owner

thanks @lrecknagel - it all seems good to me, I think let's get this merged, any extra benchmarks that could be PR'd after are always welcome but not demanded

@davidmarkclements davidmarkclements merged commit e231e2a into davidmarkclements:main Jan 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants