fix: custom fields serialization - #1375
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes a crash in community settings when deserializing custom fields that contain arrays of primitive values (e.g., multi-select dropdowns producing string[]), by preventing __key injection into non-object values.
Changes:
- Guard
value.__keyassignment to only run for non-null objects indeserialize()
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
fa07f33 to
0c58f84
Compare
| // Add __key if i is passed i.e is an array. This is needed because of ArrayField | ||
| // internal implementation | ||
| if (i) value.__key = i; | ||
| if (i !== null && i !== undefined && typeof value === "object" && value !== null) |
There was a problem hiding this comment.
sorry, this is a bit unclear for me, do we want to apply this only for array fields? If it is an array, only why not to specifically check only if it is array? not sure I understand why we check if this is an object too
There was a problem hiding this comment.
Hi Karolina, sure I will try to explain a bit more. The issue here is that if I have a custom field that is an array i.e. custom_fields.smth=["smth", "smthelse"], this is a valid use case for example dropdown wiith multiple options, the code crashes when you open the community profile page during deserialization. It is because it is expecting that this array looks like array of objects explicitly i.e. something like this is expected to be used in case you have array https://github.com/inveniosoftware/react-invenio-forms/blob/7d53be2c1bb74c5dc00362e275f0b9b88403eac1/src/lib/forms/ArrayField.js#L54 and array of strings is not accounted for. So current code, sees array and then tries to assign __key to this array member, but it is a string. The change is directed in such a way to do this, only if it is an object, which in my opinion is the case where you want to do this. Please let me know if I cleared it up or I made it worse :) thank you
There was a problem hiding this comment.
thanks for explaining, now I understand.
I think maybe it needs some more explanation in the comment above then, since we are adjusting the code to a new condition
and nitpick: I would simplify the conditional statement using one condition for the iterator, makes it more readable
// only assign __key when we're iterating an array (Number.isInteger(i)) and the element is a plain object (not a string or other
primitive).
if (Number.isInteger(i) && typeof value === "object" && value !== null)
0c58f84 to
461a29f
Compare
❤️ Thank you for your contribution!
Description
Custom field serializer unselectively treats all arrays as array fields and tries to use __key key. However, in reality you can have a dropdown with multiple options, which is just a string of arrays. Then it tries to add __key to a string and the app crashes and your communities settings page is no longer accessible for this community.
This fix addresses this problem. Please let us know if it would be OK to merge.