esmero / strawberryfield

A Field of strawberries
GNU Lesser General Public License v3.0
10 stars 5 forks source link

Refactor DataReference Dynamic Properties in Strawberryfield Item Class to a complex data/list #146

Closed DiegoPino closed 3 years ago

DiegoPino commented 3 years ago

~What is the problem?~

(no problem anymore.. sorry! see https://github.com/esmero/strawberryfield/commit/92dd91ddbc8f1dd5d9d82d9154f9191d6ee1423b0, will close once ISSUE-143 is merged and deployment configs are updated for RC2 since the Solr field settings for this little puppies are not longer ss_* solr fields)

See #144 and #143

Its actually a complex one, I stumbled across a few months ago and now is hunting me. I will need some discussions and help to find a suitable solution. The problem in concrete is that right now we expose entity reference properties of a SBF data dynamically like this

https://github.com/esmero/strawberryfield/blob/2a915a644bc3e153dcc31c0ecacf9622783f8482/src/Plugin/Field/FieldType/StrawberryFieldItem.php#L176-L187

This works fine for a single Entity (ID or Scalar stored), but does not provide the an actual List of Entities, because the the DataReferenceDefinition class does not extend nor implement a list. Even if our actual Processor class

https://github.com/esmero/strawberryfield/blob/2a915a644bc3e153dcc31c0ecacf9622783f8482/src/Plugin/DataType/StrawberryEntitiesViaJmesPathFromJson.php#L16

extend the ItemList class and can, a DataReferenceDefinition can simply not provide a list of DataReferences since it's already atomic.

The simple solution (guess what.. won't work?) would be of course to create a

https://github.com/esmero/strawberryfield/blob/2a915a644bc3e153dcc31c0ecacf9622783f8482/src/Plugin/Field/FieldType/StrawberryFieldItem.php#L190-L192

That instead of using a string as base "type" uses entity_reference core base type. But, but. That works only in Drupal theory, not in how Search API can not do that (to my late Wednesday knowledge and I know I failed before in this, but maybe I'm wiser now?)

See https://git.drupalcode.org/project/search_api/-/blob/8.x-1.x/src/Form/IndexAddFieldsForm.php#L273-337 This is where the Search API decides which properties qualify to be indexed. You will notice (if you debug this) the properties need to be either complex (so have nested properties) or be of lists/single basic types in this set:

"field_item:string_long.string" => "text"
  "field_item:text_long.string" => "text"
  "field_item:text_with_summary.string" => "text"
  "search_api_html" => "text"
  "search_api_text" => "text"
  "string" => "string"
  "email" => "string"
  "uri" => "string"
  "filter_format" => "string"
  "duration_iso8601" => "string"
  "field_item:path" => "string"
  "integer" => "integer"
  "timespan" => "integer"
  "decimal" => "decimal"
  "float" => "decimal"
  "date" => "date"
  "datetime_iso8601" => "date"
  "timestamp" => "date"
  "boolean" => "boolean"
  "language" => ""

So. What we need is to first 1.- Allow Search API to see our property as actually multiple values (like we do) but still allow internally to handle each Item as a datarefence item 2.- Look at/debug here https://git.drupalcode.org/project/search_api/-/blob/8.x-1.x/src/Utility/FieldsHelper.php#L375-383 and in general that whole class, where the actual retrieving of data is happening.

In any of the cases, the issue is simply what Search API considers "indexable" or not.

Discussion, why does this not happen in a normal Field?

Because the Many or Single value question is further down the property path. If this were a single Entity Reference Field, the field itself would expose deltas, not as properties, but before accessing the properties. Of course multi values Strawberryfields have deltas, but we are doing something different.

I know there is a solution and I'm pretty sure its not a hard one. But it requires debugging, discussion and planning. The solution will be a mix of satisfying the Search API selection requirements for indexing + implementing the right List. You will also notice that there is NO native ListDataReferenceDefinition class we could use to get around this. That is kinda sad of course.

@pcambra you willing to have a talk about this? I feel this may require some discussion and as less as overriding as possible. @patdunlavey please let me know your ideas here/questions

In any case we will get this.

DiegoPino commented 3 years ago

Was about to go to bed with this thing stuck in my brain. And then I realized the solution may be as simple was extending two interfaces, ListItemInterface and ComplexDataInterface. By doing so, we may get the best of both worlds, Search API seeing the actual core data as a complex, but also being able to list/get multiple values of DataReference type? This will allow me at least to sleep tonight.

DiegoPino commented 3 years ago

Also, right now, the multiple values are actually working ! But the issue is the because Search API sees this as single valued field it fails to index, look @patdunlavey

Drupal\search_api_solr\SearchApiSolrException while indexing item entity:node/26:en: Solr endpoint http://esmero-solr:8983/ bad request (400). ERROR: [doc=5qc1nk-default_solr_index-entity:node/26:en] multiple values encountered for non multiValued field ss_taxonomy_name: [actor, AudioChannelType] in Drupal\search_api_solr\SolrConnector\SolrConnectorPluginBase->handleHttpException() (line 1016 of /var/www/html/web/modules/contrib/search_api_solr/src/SolrConnector/SolrConnectorPluginBase.php).

DiegoPino commented 3 years ago

And fixed! gosh. Sorry for the "scare" here. All good, brain did its thing and we have multiple ones. Now back to make UI/UX better. https://github.com/esmero/strawberryfield/commit/92dd91ddbc8f1dd5d9d82d9154f9191d6ee1423b

DiegoPino commented 3 years ago

Fixed via https://github.com/esmero/strawberryfield/commit/026847c1210e18d60e5c76c21f75f62a4d8a2633