search

apostrophecms / apostrophe

A full-featured, open-source content management framework built with Node.js that empowers organizations by combining in-context editing and headless architecture in a full-stack JS environment.
https://apostrophecms.com
MIT License
4.34k stars 592 forks source link

3.0: Guard again `archived` piece field #3081

Open myovchev opened 3 years ago

myovchev commented 3 years ago

To Reproduce

Add a field with name archived to a piece scheme and try to create new piece of this type.

Expected behavior

Apostrophe should crash with a relevant message explaining this name is reserved.

Describe the bug

Creating piece failed with app crash and long stack trace not hinting the problem.

Details

OS/Browser: any

boutell commented 3 years ago

Good catch, this needs to go on the forbidden list.

On Wed, May 19, 2021 at 6:34 AM Miro Yovchev @.***> wrote:

To Reproduce

Add a field with name archived to a piece scheme and try to create new piece of this type. Expected behavior

Apostrophe should crash with a relevant message explaining this name is reserved. Describe the bug

Creating piece failed with app crash and long stack trace not hinting the problem. Details

OS/Browser: any

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/apostrophecms/apostrophe/issues/3081, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAH27KNQBWQHRKJTWOSMM3TOOH5DANCNFSM45ELUNBA .

--

THOMAS BOUTELL | CHIEF TECHNOLOGY OFFICER APOSTROPHECMS | apostrophecms.com | he/him/his

abea commented 3 years ago

I'm not sure we have a forbidden field name list somewhere right now. There are forbidden area names and this forbidden fields array. The latter one are all system generated fields, so any-doc-type triggers an error if archived is added there since it adds it to the schema. We might need a new place to put this.

Other system ones like title and slug are allowed since people might want to relabel them. Archived is difference since it's not in the UI. You could break things if you set title to a boolean field as well. Maybe we should check if those important fields are the right type and let them move on?

boutell commented 3 years ago

That's a good point, since archived is actually part of the schema it is hard to tell whether you're reconfiguring it sensibly or not, so it's hard to lint on this.

On Wed, May 19, 2021 at 10:42 AM Alex Bea @.***> wrote:

I'm not sure we have a forbidden field name list somewhere right now. There are forbidden area names @./area/index.js#L24> and this forbidden fields array @./doc-type/index.js#L466>. The latter one are all system generated fields, so any-doc-type triggers an error if archived is added there since it adds it to the schema. We might need a new place to put this.

Other system ones like title and slug are allowed since people might want to relabel them. Archived is difference since it's not in the UI. You could break things if you set title to a boolean field as well. Maybe we should check if those important fields are the right type and let them move on?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/apostrophecms/apostrophe/issues/3081#issuecomment-844172145, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAH27P25MLUBD4ECSCTS5LTOPE4DANCNFSM45ELUNBA .

--

THOMAS BOUTELL | CHIEF TECHNOLOGY OFFICER APOSTROPHECMS | apostrophecms.com | he/him/his

boutell commented 3 years ago

Maybe more of a documentation issue.

On Wed, May 19, 2021 at 11:16 AM Tom Boutell @.***> wrote:

That's a good point, since archived is actually part of the schema it is hard to tell whether you're reconfiguring it sensibly or not, so it's hard to lint on this.

On Wed, May 19, 2021 at 10:42 AM Alex Bea @.***> wrote:

I'm not sure we have a forbidden field name list somewhere right now. There are forbidden area names @./area/index.js#L24> and this forbidden fields array @./doc-type/index.js#L466>. The latter one are all system generated fields, so any-doc-type triggers an error if archived is added there since it adds it to the schema. We might need a new place to put this.

Other system ones like title and slug are allowed since people might want to relabel them. Archived is difference since it's not in the UI. You could break things if you set title to a boolean field as well. Maybe we should check if those important fields are the right type and let them move on?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/apostrophecms/apostrophe/issues/3081#issuecomment-844172145, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAH27P25MLUBD4ECSCTS5LTOPE4DANCNFSM45ELUNBA .

--

THOMAS BOUTELL | CHIEF TECHNOLOGY OFFICER APOSTROPHECMS | apostrophecms.com | he/him/his

--

THOMAS BOUTELL | CHIEF TECHNOLOGY OFFICER APOSTROPHECMS | apostrophecms.com | he/him/his

myovchev commented 3 years ago

Just to add more context - I was using archived as boolean in a piece. I got a very long scary error trace on piece create claiming the system can't replace something. Renaming the field fixed the issue. I'm not sure why, but even when I replace the archived as the same field type everything falls apart.

I've never tried to override title or slug.