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.

Sign up for GitHub

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

Jump to bottom

Fix to just project the exact fields are declared on columns and extraFields #454

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix to just project the exact fields are declared on columns and extraFields #454

wants to merge 1 commit into from

Conversation

ricaragao
Copy link
Member

Tabular has an amazing feature that is take care of the publications to just publish the fields are defined in columns and extraFields.
But there was an issue when you have a document/object with sub-documents, because the code
fields[cleanFieldName(data)] = 1;

execute cleanFieldName function to set what fields should be projected. When you have subdocuments and per example you set a column data with parent.child, cleanFieldName will get just the parent, then if you have a big document, all that fields will be published. This can be a security issue too.

I changed that line to doesn't clean the the field name, than unnecessary fields will be published.
fields[data] = 1;

This simple change will save a lot of traffic but can have a little negative impact on systems that are using another subdocuments that are not explicit published. Anyway, I guess this is a important change.

@ricaragao ricaragao linked an issue May 18, 2024 that may be closed by this pull request
Remove autopublish for sub-documents #453
Open
@lynchem
Copy link
Collaborator

lynchem commented May 30, 2024

Hey @ricaragao , sorry about taking so long to reply. I think this is a better way of doing it. I only have two concerns:

  • if someone is using this and inadvertently relying on getting more fields
  • if they have another publication active on the collection I think merge-box can't handle subfields. So if you ask for config.a here and in another ask for config.b it will overwrite it.

Maybe we can make it opt-out ? So a config option saying cleanFieldName which defaults to true and you can set it in the tabular to turn it off ?

@ricaragao
Copy link
Member Author

Hey @ricaragao , sorry about taking so long to reply. I think this is a better way of doing it. I only have two concerns:

  • if someone is using this and inadvertently relying on getting more fields
  • if they have another publication active on the collection I think merge-box can't handle subfields. So if you ask for config.a here and in another ask for config.b it will overwrite it.

Maybe we can make it opt-out ? So a config option saying cleanFieldName which defaults to true and you can set it in the tabular to turn it off ?

Hi @lynchem , yes, can be a good idea use a opt-out. Do you think it can be on main configuration object?

@lynchem
Copy link
Collaborator

lynchem commented May 30, 2024

Hi @lynchem , yes, can be a good idea use a opt-out. Do you think it can be on main configuration object?

Yup, sounds good 👍🏻

@jankapunkt jankapunkt changed the base branch from devel to master July 8, 2024 14:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jankapunkt jankapunkt Awaiting requested review from jankapunkt

@lynchem lynchem Awaiting requested review from lynchem

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove autopublish for sub-documents
2 participants
@ricaragao @lynchem