Add privateAttributes to global and per model response

[dalbitresb12]

Description of what you did:

Restarting the work made from PR's #6002 and #4914. Also added the option to ignore the global private attributes per model using options.ignoreGlobalPrivateAttributes. Finally, new tests were added for sanitizeEntity.

Fixes #4342.

(I previously opened another PR for the same (#7330), but I forgot the sign for DCO. Force-pushing didn't seem to work, so I opened a new one.)

[codecov]

Codecov Report

Merging #7331 into master will increase coverage by 0.04%.
The diff coverage is 76.19%.

@@            Coverage Diff             @@
##           master    #7331      +/-   ##
==========================================
+ Coverage   27.16%   27.21%   +0.04%     
==========================================
  Files        1163     1164       +1     
  Lines       15518    15519       +1     
  Branches     2410     2411       +1     
==========================================
+ Hits         4216     4223       +7     
+ Misses       9534     9528       -6     
  Partials     1768     1768              

Flag	Coverage Δ
#front	19.36% <63.63%> (+0.04%)	⬆️
#unit	53.88% <90.00%> (+0.02%)	⬆️

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

Impacted Files	Coverage Δ
...admin/admin/src/components/ContainerFluid/index.js	66.66% <ø> (ø)
...in/src/components/LeftMenu/LeftMenuFooter/index.js	33.33% <0.00%> (-33.34%)	⬇️
...rc/containers/EditViewDataManagerProvider/index.js	0.00% <0.00%> (ø)
.../admin/src/components/ComponentIconPicker/index.js	0.00% <0.00%> (ø)
...agerProvider/utils/retrieveComponentsFromSchema.js	68.75% <ø> (ø)
...rapi-plugin-upload/admin/src/translations/index.js	0.00% <ø> (ø)
packages/strapi-provider-upload-local/lib/index.js	29.16% <0.00%> (ø)
...ovider/utils/getFieldsActionMatchingPermissions.js	100.00% <100.00%> (ø)
...ditViewDataManagerProvider/utils/tests/testData.js	100.00% <100.00%> (ø)
...rc/containers/InputModalStepperProvider/reducer.js	68.33% <100.00%> (ø)
... and 3 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bd29fe7...4a25726. Read the comment docs.

[dalbitresb12]

Any chance of getting a review before there are more conflicts? Thank you. @Convly

[Convly]

Looking globally good.
Some questions/concerns about the ignoreGlobalPrivateAttributes attribute.

[dalbitresb12]

Changes done! Hope it's all OK. @Convly

[Convly]

Almost there!

[dalbitresb12]

Hello @Convly. I've done the changed you requested.

What about when attribute.private === true but the attribute has been ignored in ignorePrivateFor array? Here it would set the isPrivate boolean to true (because of the ||), but I would expect it to be set to false (because it has been explicitly ignored)

I decided to add another helper for the ignored private attributes (getIgnoredPrivateAttributes) because this value was also needed in the shouldRemoveAttribute helper to force the attribute to be ignored in the case you mentioned. I hope that's ok, but I'm not sure if it's the best way to do this.

About the GraphQL plugin, I've never used GraphQL. So I've been guessing how the function works thanks to the way it was done on the previous PR (#6002). Also, there was no tests for this code, so I had no way to test it. I would appreciate if you could tell me if everything is ok. I'll try to read more about GraphQL and I'll try to test if this code works as it should.

[dneckel]

Would love to see this feature merged as without it there is often too much unnecessary response data and insights (when was something created/updated) especially with auto populating relationships.

[Convly]

It looks good to me. Thanks again for your contribution, we really appreciate that!

However, before merging this, I would like to have feedback from @alexandrebodin both on ignorePrivateFor and the GraphQL modifications.
Even if I can't give you a precise ETA for this review, it should be during next week.

I hope you can understand, have a good day! 🙂

[dalbitresb12]

Hope it gets merged soon! Would be really helpful. Have a good day! :D

[derrickmehaffy]

Hope it gets merged soon! Would be really helpful. Have a good day! :D

Alex is currently out of the office until next week, after which I'm sure he will get some time to review it. 😃

[dalbitresb12]

Hi @Convly @derrickmehaffy, are there any updates on this? Thank you.

[derrickmehaffy]

Sorry Alex had a lot to catch up on, if he doesn't respond by tmrw I'll try and poke him ;)

[alexandrebodin]

Hi, This looks pretty good but I think there is one point I want to change to simplify the usage.

I think we should remove the ignorePrivateFor option. I don't see a good reason to set a global if it is not use globally. IMO it is better to add fields locally (for readability and maintenability) than adding them globally and excluding them locally.

One more thing: I would just like to see the utils in the ContentType utils in strapi-utils to be reused in graphql and in the core instead of having to duplicate the logic :)

Thanks for the work !

[dalbitresb12]

Hi @alexandrebodin,

I think we should remove the ignorePrivateFor option. I don't see a good reason to set a global if it is not use globally. IMO it is better to add fields locally (for readability and maintenability) than adding them globally and excluding them locally.

I do agree with you that it is better to add fields locally only, but it might happen that someone might need to exclude an specific global private attribute in only 1 model. This wouldn't follow the DRY principle, as you would end up adding that attribute to all the other models individually. That's just my opinion, but if you believe it should be removed I'll do it.

One more thing: I would just like to see the utils in the ContentType utils in strapi-utils to be reused in graphql and in the core instead of having to duplicate the logic :)

I didn't know that was possible. How should I import the sanitizeEntity helper into the GraphQL plugin? Maybe something like this:

const { sanitizeEntity } = require('strapi-utils');

Also, what do you mean with in the core? Do you mean the Strapi core?

[alexandrebodin]

Hi @alexandrebodin,

I think we should remove the ignorePrivateFor option. I don't see a good reason to set a global if it is not use globally. IMO it is better to add fields locally (for readability and maintenability) than adding them globally and excluding them locally.

I do agree with you that it is better to add fields locally only, but it might happen that someone might need to exclude an specific global private attribute in only 1 model. This wouldn't follow the DRY principle, as you would end up adding that attribute to all the other models individually. That's just my opinion, but if you believe it should be removed I'll do it.

For now I would rather not add too many options :)

One more thing: I would just like to see the utils in the ContentType utils in strapi-utils to be reused in graphql and in the core instead of having to duplicate the logic :)

I didn't know that was possible. How should I import the sanitizeEntity helper into the GraphQL plugin? Maybe something like this:

const { sanitizeEntity } = require('strapi-utils');

Also, what do you mean with in the core? Do you mean the Strapi core?

You can export the utils like getIgnoredPrivateAttributes from strapi-utils so you can import them in the graphql plugin yes :) When I'm talking about the core I'm refering to the strapi package but in this case you won't have much to do there

[dalbitresb12]

@alexandrebodin I'll work on the changes this weekend and it should be ready for review on Monday.

[dalbitresb12]

Hi @alexandrebodin. I've done the changes you requested. Is there anything else missing to merge this PR?

[dalbitresb12]

Hi, @alexandrebodin. Have you had any chance to review the latest changes? Is there anything else needed?

[alexandrebodin]

@derrickmehaffy can you check if the doc is good for you ?

[derrickmehaffy]

This looks awesome, docs are great and I am already seeing a spot we can set the default limit now also in ./config/api.js :P

[alexandrebodin]

LGTM

[dneckel]

thank you @dalbitresb12 for implementing this feature and sticking with it 🚀

[dalbitresb12]

@alexandrebodin Do you have an approximate release date for 3.2.0?

[Convly]

Hello @dalbitresb12, 3.1.6 has been released and includes the privateAttributes option 🎉
Here is the changelog https://github.com/strapi/strapi/releases/tag/v3.1.6

[lauriejim]

This pull request has been mentioned on Strapi Community. There might be relevant details there:

https://forum.strapi.io/t/privateattributes-not-working-in-my-model/171/2