Skip to content

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

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

Closed
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
@ricaragao ricaragao requested review from jankapunkt and lynchem May 18, 2024 16:10
@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
@ricaragao
Copy link
Member Author

@lynchem and @jankapunkt , I created a new PR following the @lynchem suggestion and point it to the migration version branch.

I'm closing this PR.

@ricaragao ricaragao closed this Jul 13, 2025
@ricaragao ricaragao deleted the project-fields branch July 13, 2025 23:40
@jankapunkt
Copy link
Member

@ricaragao was it outdated or closed due to no activity? I currently don't actively use tabular so I had no reference project for testing.

@ricaragao
Copy link
Member Author

@jankapunkt , not exactly. I just did the same modification in another branch with a new option where the user can choose if they want this behavior or not. The default will be as before. This new branch I did a PR with target to the migration branch.
You can see that it is a PR with more information: #466

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.

Remove autopublish for sub-documents
3 participants