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

fix(db-postgres): Remove duplicate keys from response #4747

Merged
merged 3 commits into from
Jan 16, 2024

Conversation

paulpopus
Copy link
Contributor

Closes #4709

Description

  • I have read and understand the CONTRIBUTING.md document in this repository.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • I have added tests that prove my fix is effective or that my feature works
  • Existing test suite passes locally with my changes

if ('name' in field && Boolean(field?.name)) {
const fieldToDelete = `${groupFieldPrefix}${field.name}`

deletions.push(() => delete ref[fieldToDelete])
Copy link
Contributor

Choose a reason for hiding this comment

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

As I read this I realize that what we talked about here isn't quite right.

Instead of having the loop on the fields at this iteration, we should be deleting the fields if there is a fieldPrefix when we iterate on the field.

If we do it this way we can be sure that fields nested in rows and collapsibles are also deleted from the parent. The way this PR is written we wouldn't be catching those.

Copy link
Contributor

@DanRibbens DanRibbens left a comment

Choose a reason for hiding this comment

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

I mislead you on the call we had. We need to push to deletion in the existing reduce function at line 115 if there is a fieldPrefix. This way the extra data will be removed if you have group -> row -> text for example.

@paulpopus
Copy link
Contributor Author

Is there a situation where fieldPrefix is expected on the field though? If we do a blanket if fieldPrefix => delete are there edge or configurations cases we're missing?

@DanRibbens
Copy link
Contributor

Is there a situation where fieldPrefix is expected on the field though? If we do a blanket if fieldPrefix => delete are there edge or configurations cases we're missing?

I don't think so? The resulting doc should nest all properties into the field structure defined in the config. Prefixes aren't a thing in that context.

I assume our test coverage will tell us pretty quickly.

@paulpopus
Copy link
Contributor Author

I mislead you on the call we had. We need to push to deletion in the existing reduce function at line 115 if there is a fieldPrefix. This way the extra data will be removed if you have group -> row -> text for example.

I updated it at 118 and it passes the test coverage.

I also noticed that _order was not intended to leak out based on it being removed from some of the objects so I remove that as well in some areas such as 166.

Sorry the linter wanted to reorder objects and imports all over the file so the diff is a bit miffed.

@DanRibbens DanRibbens merged commit eb9e771 into main Jan 16, 2024
30 checks passed
@DanRibbens DanRibbens deleted the fix/4709-fix-duplicate-data-leaking-out-of-postgres branch January 16, 2024 18:22
@DanRibbens
Copy link
Contributor

I believe this also fixed #4221

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.

Duplicate fields in API response when using group fields within an an array field
2 participants