agentejo / cockpit

Add content management functionality to any site - plug & play / headless / api-first CMS
http://getcockpit.com
MIT License
5.39k stars 523 forks source link

[bug] Importing "object-structured" fields sets fields to `null` #1464

Open abernh opened 2 years ago

abernh commented 2 years ago

The bug

When exporting and importing a collection with "complex" field types like asset or set via the UI, those "object-structured" fields values are not imported but set to null.

Steps to reproduce

Note: only tested with JSON

Point of bug origin

The problem starts on doImport when the parsed data is once more re-parsed to be then sent to Import::execute. For any fields that have an object instead of a simple text value it expects a type key that is then compared with the field.type. And that type key is completely missing.

https://github.com/agentejo/cockpit/blob/568b0124352f6d27df359e8c19a70d2dd1961e87/modules/Collections/views/import/collection.php#L330

Possible fix

This particular check was added in 97bb6b6 in order to allow the importing of multiple links (says the commit message). It was later adjusted to ignore arrays (and let them pass un-checked) 7fdd136 I was not able to determine if this check for the type key was ever working, as by default no field seems currently to export a type key - also not collection-links (which are currently an array).

Options

(1) Removing the the check

.... and "trust" the mapping process that the imported value (string, object, array) is the expected one.

(2) Adding the type key into the export

The alternative is to add the missing type key.

(2A) Either by adding it to all field types and moving the value into value

Pro: consistency Con: adds a lot of boilerplate to the export making the export itself not that useful for anything else but importing.

[
  {
   _id: ...,
   textfield1: {
     type: 'text',
     value: 'this is the value' 
   },
   setfield2: {
     type: 'set',
     value: {
       subfield1: 'some more text',
       subfield2: 'and more text'
     }
   },
   asset3: {
     type: 'asset',
     value: { ... assetobj ... }
   },
   ...
]

(2B) as a "private" key _type to all fields with an object as value

Pro: less boilerplate than 2A Con: _type would be the only "meta"-key in the whole export

[
  {
   _id: ...,
   textfield1: 'this is the value',
   setfield2: {
     _type: 'set',
     subfield1: 'some more text',
     subfield2: 'and more text'
   },
   asset3: {
     _type: 'asset',
     ... assetobj ... 
   },
   ...
]

(2C) Extending the export format to be more like collections/entries/

meaning it is split in a fields and a data part, where fields will hold the type information for the to be expected values in data

{
 fields: ...,
 data: [...]
}

Pro: hardly any boilerplate or "meta" fields Con: changes export format leads to backwards compatibility issues

Suggestion

My suggestion is to go for (1) and treat object-type values like any other.

Notes

Adjustments to the export would needed to be done in the Collections\Controller\Admin->export function after the entries are retrieved and before they are pretty-jsonfied. https://github.com/agentejo/cockpit/blob/568b0124352f6d27df359e8c19a70d2dd1961e87/modules/Collections/Controller/Admin.php#L420

Adjustments to the import (2C) are in modules/Collections/assets/import/parser.js

Possible workaround (*not tested)

Disable lines 330,331 in the cockpit source if you trust your import-data-structure.

Related

https://discourse.getcockpit.com/t/import-set-field-type-not-working/2052/2

Ciseur commented 2 years ago

My 2 cents: solution (2) won't work for an import of data not coming from a cockpit export.

I am currently trying to import a list of cities from an open data and the {"lat":__, "lng":___} object does not get imported. And I don't feel like solution 2 would help me.

Ciseur commented 2 years ago

I just tryed the suggested workaround : disabling line 330 and 331 on cockpit/modules/Collections/views/import/collection.php : it works!