drupal-graphql / graphql-search-api

GraphQL & Drupal Search API integration.
10 stars 22 forks source link

Incorrect handling of emty-like field values #21

Closed bucefal91 closed 5 years ago

bucefal91 commented 5 years ago

We are using this cool module to expose one of our Search API indexes to our front end.

It works really cool and has saved us days of hard work :) Just now that we are wrapping up the integration with Search API, I discovered a minor bug.

graphql_search_api would show NULL in the result for any value that looks like empty, i.e. empty($value) === TRUE (in PHP code).

The way I discovered it was the following, we had an integer field in the index and for many documents it would be 0. For such documents the graphql query would return null instead of the number.

Consider the following example. First, let me demonstrate my solr index - you will see it has the field its_kaltura_views_30d defined for multiple documents it is 0:

$ curl 'http://solr:8983/solr/drupal/select?q=index_id:kaltura&indent=on&fl=its_kaltura_mid,its_kaltura_views_30d'
{
  "response":{"numFound":6,"start":0,"docs":[
      {
        "its_kaltura_views_30d":15,
        "its_kaltura_mid":2},
      {
        "its_kaltura_views_30d":2,
        "its_kaltura_mid":3},
      {
        "its_kaltura_views_30d":0,
        "its_kaltura_mid":4},
      {
        "its_kaltura_views_30d":0,
        "its_kaltura_mid":5},
      {
        "its_kaltura_views_30d":0,
        "its_kaltura_mid":6},
      {
        "its_kaltura_views_30d":0,
        "its_kaltura_mid":7}]
  }}

Now if I request the same data over graphql endpoint, for non-zero I will see the correct number whereas all the zeros are replaced with NULL: Screenshot from 2019-07-13 12-55-24

duartegarin commented 5 years ago

Hi @bucefal91 , Apologies for the late reply, it's been hectic! I'll have a look at your PR soon and merge it if it's all in order. Thanks again!

bucefal91 commented 5 years ago

No problem :) I perfectly understand it. :) Thanks for this nice module, it did save us days of hard work! Contributing small patch is the least I could do in return :)

duartegarin commented 5 years ago

@bucefal91 I have refactored things a bit in PR #23 and it should also fix this problem. I'm closing this but if you want to test the other PR would be amazing :) Thanks again for flagging this 👍