Add CustomResource module and Fix Marshalling

[dlitvakb]

Fixes #79
Fixes #80

[neonichu]

Should we warn/fail if client == nil or the content type isn't cached, yet? Might be better than silently not having any fields.

[dlitvakb]

@neonichu The entry will still have fields, just not dynamically assigned.

And can be read via entry.fields[:field_name]

Also, the only case when client == nil is when you create the entry manually like MyCustomEntry.new, in which case you can provide a hash as a parameter with the values for the fields, which will be available even if the CT is not cached.

Only possible exception is: Client exists, CT not cached, that case can happen under 2 conditions:

1. Client created before CT was and no client reload happened
2. More than 1000 CTs in the Space and entry was requested by ID and is of one of the CTs left out

[neonichu]

Second case for content types can't happen because there's a limit of 50

Considering we think both cases are impossible, making it straight up throw seems better?

[dlitvakb]

I don't think raising an exception is a good idea, as user might not have :dynamic_entries activated (which should be default, but that's a different discussion) and therefore CTs not cached at all.

If that's the case, then having to operate over :fields is the expected fallback behavior

[pxlpnk]

I would suggest documenting this behaviour then.

[dlitvakb]

The CT cache behaviour is now removed and only relies on defined Custom Resource schema

[toadle]

So, I tried it out.

The property-thing seems to work now:

class Contentful::Category < Contentful::BaseEntry  
  property :title
  ...
end

Where:

class Contentful::BaseEntry < Contentful::Entry
  include Contentful::Resource::CustomResource
  ...

The sad thing is that properties have no setters (e.g. category.title=) in this case. Meaning I can't use the usual factory_girl-syntax to create something to test with:

FactoryGirl.define do
  factory :category, class: Contentful::Category do
    title       { FFaker::DizzleIpsum.word }
    slug        { FFaker::Internet.slug.gsub(".","") }
  end
end

Any chance that you guys would add that "officially" or that I might monkey-patch it in?

I tried something like this:

    properties.keys.each do |name|
      define_singleton_method "#{Contentful::Support.snakify(name)}=".to_sym do |value, wanted_locale = default_locale|
        properties[name] = value
        fields(wanted_locale)[name] = value
      end
    end

in update_mappings! but that didn't create any additional methods. Couldn't tell why.

[toadle]

Sadly the Marshal.dumping of contentful-objects doesn't seem to be fixed with this:

Marshal.dump client.entries("content_type" => "<content_type_id>").first
TypeError: no _dump_data is defined for class HTTP::Parser
from (pry):12:in `dump'

Perhaps there is still something lingering in raw_client.entry(id).object ?

Thanks for your support. Would be really cool if we get this to work.

[dlitvakb]

Hey @toadle,

Thanks for jumping in and looking into this.

Regarding Marshal it only works for classes defined by the user, like in the added spec, there is a Cat class mapping to the nyancat entry. You can define these through entry_mappings: { 'ct_id' => YourClass }. Sadly Marshal does not work for anonymous classes, so anything that wants to be Marshalled needs to be explicitly mapped.

Regarding the setter methods, as the Delivery API is read-only, it's not contemplated to use setters externally. I can talk with our product manager and decide if it's a good idea to add that.

You can either overwrite manually the @fields instance variable, or create the objects with the constructors first argument, which receives an API-formatted Hash.

Hope you find this answer useful,

And if you can, please let me know if there is anything that you would find important to mention in the Readme or the Class Documentation regarding these use cases that might be missing.

[toadle]

@dlitvakb I'll figure something out for that setter-methods stuff, thanks!

Regarding the marshalling. In my case, it is exactly like you said: I have a custom resource_mapping for the content_type returned by the query.

So client.entries("content_type" => "<content_type_id>").first will return a

class Contentful::BaseEntry < Contentful::Entry
  include Contentful::Resource::CustomResource
...

I saw that you guys recently moved to defining an entry_mapping where I still have an resource_mapping that figures out the class by looking at the content_type.

  def resource_mapping 
    { 
    'Asset' => ->(_json_object) do
      asset_type = _json_object.try(:[], "fields").try(:[],"file").try(:[], "details").try(:keys).try(:first)
      asset_class_for(asset_type)
    end,
    'Entry' => ->(_json_object) do
      content_type_id = _json_object.try(:[], "sys").try(:[], "contentType").try(:[], "sys").try(:[], "id")
      entry_class_for(content_type_id)
    end
    }
  end

Don't know if that is important, but all my classes are correctly mapped to something that is a Contentful::Entry / Contentful::CustomResource.

So I'd still say that there is a Parser lingering in there somewhere :-)!

BTW: Just a thought about that mashalling. If I understand the implementation correctly, then this could potentially lead the a huge load of unwilling API-requests which don't result in the same outcome, when marshal_loaded.

Let's say I requested client.entries("content_type" => "<id>", include: 2) then try to cache that result for example to fill a menu with subitems. If I see this correctly then all those included-objects will get lost during mashalling and - if my menu has 10 items - I will have 10 API-requests on my hand that are potentially slow. Correct?

Is it too difficult to strip out all Procs / Parsers and then marshal just using the ruby way?

[dlitvakb]

Hey @toadle,

I'd have to test how this works with :resource_mapping I think that part of that Proc behavior you find, might be related to it, as it goes through a Proc to figure out how which class to initialize instead of just mapping it through a hash index.

As for Raw calls not hydrating the amount of includes... I should probably add a way of telling the object how many includes to marshal.

Any hydration should retrieve the object as expected (though currently, minus the includes).

Thanks for pointing me into a better direction for improving this library 👍

[dlitvakb]

Hey @toadle,

I've been working on improving marshalling, and added handling for included objects and added specs for both ways of resource mapping + marshalling.

Please let me know if this solved your problem

PS:

Here is a little script that checks for both of this methods (this is what the specs added were based from): https://gist.github.com/dlitvakb/ca5ad8737f1d4bc5f90e

[toadle]

Hey @dlitvakb, cool stuff. Sorry for taking some time to test this, but I had to finish a story here ;-)!
Just wanted to let you know I'm back on testing this and will have feedback shortly.

[toadle]

@dlitvakb So first I checked the marshalling with my example.

Good news: marshal_dump now works. I like the keeping of the raw-response.
Bad news: Doesn't yield the same results when marshal_loading.

My scenario is this: I load a small category-tree (categories have subcategories), for being able to look up the breadcrumb for certain articles. Therefore I load the categories with include: 2.

@top_categories = client.entries("content_type" => "<id>","fields.top" => true, include: 2)

I put those into the Rails.cache like so:

@top_categories = Rails.cache.fetch("top_categories", expires_in: 12.hours) do
    client.entries("content_type" => "<id>","fields.top" => true, include: 2)
end

Works the first time around.

Second time not so much.
Somewhere in the bowels of my application I create a lookup from those categories, where is traverses categories and subcategories. The first-level categories come out fine. The second-level seems to be broken, since I suddenly get a Hash where a Category should be.

The Hash looks like this:

{"sys"=>{"type"=>"Link", "linkType"=>"Entry", "id"=>"7TnTnJvHu8yIEAoMScG4E"}}

So I suppose the marshal_loading doesn't quite do what is it supposed to do at the second level.

[dlitvakb]

Hey @toadle

Thanks for the quick answer and the interest in this PR.

Would you be willing to contribute a fix?

Your case is more complex than I expected and I need more information to continue moving on with this.

Can you provide me with the following:

• JSON Preview of the Content Type you're trying this for
• Example entries for the expected nesting
• Any other thing you have in consideration for this to work

You can fetch the data required in JSON format using contentful-bootstrap.rb with the following command:

$ contentful_bootstrap generate_json $SPACE_ID $ACCESS_TOKEN --output-file dump.json

[toadle]

Hey @dlitvakb,

sure I'd like to provide a fix. So far I haven't totally understood how the parsing-process works for the returned JSON. Therefore I haven't contributed. From what I understood you are using basic jsonapi.org -style JSONs, but the GEM already hands out neatly build object-trees which were already looked up in the links. I'll look into it closer and see if I can provide code myself.

As for the infos you requested.

JSON for the Content-Type
https://gist.github.com/toadle/52f63751c58d0cda0777

JSON for one entry
https://gist.github.com/toadle/48acfe3dbd8dbb1ae477

Since I request my entries with include: 2 there should just be more entries in the links.

(I dumped this by hacking resource_mapping, cause https://github.com/contentful-labs/contentful-bootstrap.rb would only hand out very basic fields)

I think it is pretty straight forward. Should be the same for every content-type that uses references/links.

[dlitvakb]

Hey @toadle,

Thanks for the follow up,

I'll be trying to reproduce this scenario and get back to you as soon as possible.

In the meantime, if you want to dig into the codebase I'm more than willing to give you a hand to get you familiarised.

Any kind of help is always welcome!

[toadle]

@dlitvakb Just to let you know: I got the scenario reproduced as a spec in my fork.
I'll try to fix it. Most probably not today anymore - at least "today" in GMT+1 ;-)!

[toadle]

@dlitvakb: Foxed here #83 .

[dlitvakb]

Thanks @toadle,

I'll finish testing this.

I'll reword your commits to follow the Style Guide we use and submit for review afterwards.

Thank you very much for the contribution and hope to collaborate more in the future.

[toadle]

@dlitvakb Could you help me with one question so that I can put this thing in production-use?

As you know I want to be able to use object.property= in my factories. I thought I'd just monkey-patch update_mappings! in my class Contentful::BaseEntry < Contentful::Entry like this:

  def update_mappings!
    puts "update_mappings!"
    properties.keys.each do |name|
      define_singleton_method Contentful::Support.snakify(name).to_sym do |wanted_locale = default_locale|
        properties[name] ||= fields(wanted_locale)[name]
      end

      define_singleton_method "#{Contentful::Support.snakify(name)}=" do |value, wanted_locale = default_locale|
        fields(wanted_locale)[name] = value
      end
    end
  end

Sadly this doesn't work - can't tell why. It always gives me that property= is not defined.

Where would you monkey-patch this "setter" in?

[dlitvakb]

Hey @toadle,

It's not working because of the ||= in the properties assignment (which makes total sense on a read-only api)

You can accomplish what you want by doing the following:

module SetterResource
  def update_mappings!
    properties.keys.each do |name|
      define_singleton_method Contentful::Support.snakify(name).to_sym do |wanted_locale = default_locale|
        properties[name] = fields(wanted_locale)[name]
      end

      define_singleton_method "#{Contentful::Support.snakify(name).to_sym}=" do |value, wanted_locale = default_locale|
        fields(wanted_locale)[name] = value
      end
    end
  end
end

class Cat < Contentful::Entry
  include Contentful::Resource::CustomResource
  include SetterResource

  property :name
  property :lives
  property :bestFriend, Cat
end

Hope that helps

[toadle]

@dlitvakb Sadly no. When I do it this way then properties is always empty.

I don't really get this: Is there a difference between properties and the stuff in fields ?
Why does this difference exist?

I also though about hacking Contentful::Resource#property, but since that is a ClassMethod I can't access the fields.

[dlitvakb]

Hey @toadle

Properties define the schema of the object, whereas fields include the API response.

Have you tried using the exact code I sent you? Instead of monkey patching update_mappings! using the mixin I wrote.

Here is the test I used to check it and works perfectly:

class OtherCat < Contentful::Entry
  include Contentful::Resource::CustomResource
  include SetterResource

  property :name
  property :lives
  property :bestFriend, OtherCat
end

def test_setters
  nyancat = client(entry_mapping: { 'cat' => OtherCat }).entries(include: 1, 'sys.id' => 'nyancat').first

  nyancat.name = 'blah'

  assert(nyancat.name, 'blah')
end

Please let me know if that worked

[toadle]

Yes I did. I got the slight feeling that this update_mappings! is not overwritable that way.

I did it like this:

class Contentful::BaseEntry < Contentful::Entry
  include Contentful::Resource::CustomResource
  include Contentful::SetterResource
...
end

and

class Contentful::Category < Contentful::BaseEntry
  property :title
...
end

Then tried it out:

Contentful::Category.new.title = "test"
NoMethodError: undefined method `title=' for #<Contentful::Category:0x007f95dc136220>
from (pry):2:in `<main>'

Tried to debug it afterwards:

   2: def update_mappings!
   3:   binding.pry
 =>4:   properties.keys.each do |name|

Result:

[1] pry(#<Contentful::Category>)> properties
=> {}
[2] pry(#<Contentful::Category>)> properties.keys
=> []

So in the end the goal is that properties hold all attributes of the object, right?
They might be mapped to fields.

I couldn't find the place where the attributes in properties are set.

[dlitvakb]

Hey @toadle,

I found what's causing the properties hash to be empty.

I'll update that later today and get you up and running

[toadle]

@dlitvakb Any update on this? Otherwise I have this PR already in use ;-)

[dlitvakb]

Hey @toadle

Sorry for the delay,

I was working on an integration project.

The fix has now been pushed

Please let me know if that helps

[toadle]

@dlitvakb OK, got it working. Some little changes still needed, but this works:

module Contentful::SetterResource
  def update_mappings!
    @fields = {} if @fields.nil?

    properties.keys.each do |name|
      define_singleton_method Contentful::Support.snakify(name).to_sym do |wanted_locale = default_locale|
        @fields[wanted_locale] = {} if fields(wanted_locale).nil?
        properties[name] = fields(wanted_locale)[name]
      end

      define_singleton_method "#{Contentful::Support.snakify(name).to_sym}=" do |value, wanted_locale = default_locale|
        @fields[wanted_locale] = {} if fields(wanted_locale).nil?
        fields(wanted_locale)[name] = value
      end
    end
  end
end

and having everything be a Contentful::BaseEntry.

class Contentful::BaseEntry < Contentful::Entry
  include Contentful::SetterResource
...

Thanks for the help up to here!

[dlitvakb]

No problem @toadle

Glad to help