Contentful.rb breaks if a content type has a field called 'locales'

[felixjung]

Hey,

Today, I updated the Jekyll Contentful plugin which depends on this project. After the update the data import from Contentful was broken. I would always receive an error

undefined method "each" for nil:NilClass

which of course tells you, you're trying to loop over a collection, when in fact you don't have a collection, but rather nil. Later I also encountered the variation

undefined method "each" for "fr-FR":String

After spending quite some time debugging this, I found that the error stemmed from #106, specifically these lines of code:

res.locales.each do |locale|
  property_containers << res.fields(locale)
end

inside ResourceBuilder#replace_links_with_known_resources.

Long story short, it turned out that from earlier attempts at hacking together localization for our content, several of our content types had a disabled field called locales that was configured as type short text and sometimes was empty or contained a comma separated list (not a list) of locales. The field was not configured to be an array/list so it parsed as a single string.

Disabling these dormant fields in responses for all content types fixed the issue, eventually. I'm not sure how the official implementation of locales is handled within Contentful and in particular this library. For example, whether the locales are added on an entry level or within the sys property. However, unless you have introduced some restrictions on field names for content types, the current implementation of localization in this library is not safe.

What do you think?

[dlitvakb]

Hey @felixjung,

As always, thanks for your contributions.

This is indeed a serious issue, if you found a temporary solution, that's awesome.

That said, I noticed this issue too very recently, as I just finished implementing the Python equivalent SDK, and locale management is done very differently than here, in a more simplified way, which solves this kind of issues, along with other issues discovered in this SDK, which make it extremely complex and prone to errors on some edge-cases.

This has prompted us to look forward to a complete rewrite of the SDK (keeping compatibility, but with a fresh source). I'll push forward this issue as another reason to speed up the rewrite, as this gives further reasons to go for it.

Cheers

[felixjung]

👍

[felixjung]

Today, I ran into a similar problem. I had created a content type called Website with the following JSON structure:

{
  "name": "Website",
  "description": "The website this content should be available on, including subdomains for multi-language countries.",
  "displayField": "domain",
  "fields": [
    {
      "name": "Domain",
      "id": "domain",
      "type": "Symbol",
      "required": true,
      "validations": [
        {
          "unique": true
        },
        {
          "regexp": {
            "pattern": "^(?:(?:[a-z]{2}-)?[a-z]{2,}\\.)?sumup\\.[a-z]{2,}(?:\\.[a-z]{2,})?$",
            "flags": "g"
          },
          "message": "This needs to be a valid SumUp domain, with optional sub-domain."
        }
      ],
      "localized": false,
      "disabled": false,
      "omitted": false
    },
    {
      "name": "Country",
      "id": "country",
      "type": "Symbol",
      "required": true,
      "validations": [
        {
          "unique": true
        },
        {
          "regexp": {
            "pattern": "^[A-Za-z ]+$",
            "flags": "g"
          }
        }
      ],
      "localized": false,
      "disabled": false,
      "omitted": false
    },
    {
      "name": "Locale",
      "id": "locale",
      "type": "Symbol",
      "required": true,
      "validations": [
        {
          "regexp": {
            "pattern": "^[a-z]{2,}-[A-Z]{2,}$",
            "flags": "g"
          },
          "message": "The locale needs to be a valid ISO locale."
        }
      ]
    }
  ],
}

The idea was to allow content creators to specify the company websites (domains) an entry of other content types (for example Landing Page) should be available on by adding entries of the Website content type as references.

When I tried to pull our space using the Jekyll data importer, specifying the locale "en-GB" in the cda_query option, contentful.rb would end up crashing due to a stack overflow:

SystemStackError: stack level too deep
  /usr/local/lib/ruby/gems/2.3.0/gems/contentful-1.1.0/lib/contentful/dynamic_entry.rb:36:in `block (3 levels) in create'
  /usr/local/lib/ruby/gems/2.3.0/gems/contentful-1.1.0/lib/contentful/resource/fields.rb:14:in `fields'

After deleting the "Locale" field on the Website content type, the error disappeared. I guess when creating dynamic entries, the code somehow ends up in a circular reference when trying to pick the localized versions of fields. Does this make sense or might there be another issue at hand? I've since then replaced the Locale field with a Site Locale, which works just fine.

I guess you can put this on the list of issues with the library?

Thanks 😄

[dlitvakb]

Yes, this is happening because the library is looking for sys.locale using a shortcut that then gets overwritten by your field. This is currently a known issue.

Next week I'm going to be in the office and the matter of an SDK re-write is going to be debated during planning on Monday.

I'll keep you posted.

Cheers

[dlitvakb]

Work on this has been started on #113

Cheers

[dlitvakb]

@felixjung #113 is mostly done, wanna check it out?

Cheers

[felixjung]

Thanks @dlitvakb. I will take a look once I'm done with some stuff on the contentful-batch-libs 😉

[felixjung]

Yay 👍

[dlitvakb]

Hey @felixjung,

The 2.0.0 version was merged to master, and not yet released.

We're looking for some real user feedback, would you like to try it out? If so, please add your feedback here: https://github.com/contentful/contentful.rb/issues/120

Cheers!

[felixjung]

Hey @dlitvakb, I would really like to, but I'm working on some other things right now (upgrading content model, migrating content between spaces, fun... 😕) and I don't know when I can make time to test it.

Also, note we're only using this through the Jekyll integration. Judging by the list of breaking changes, I assume I could just upgrade the dependency in the contentful-data-import plugin to use the 2.0.0 branch from Github? Would that work?

[dlitvakb]

Yup, it should work, as none of those are affecting the plugin.

Cheers