Raise meaningful error when no serializable class defined for resource

[smaximov]

This PR introduces a better error message than cryptic undefined method 'new' for nil:NilClass when no serializable class defined for a resource.

[smaximov]

Just got similar "undefined method" error here: https://github.com/jsonapi-rb/jsonapi-serializable/blob/f7f1ac88ebb41b3b1b23d06f5bb672b9c10a5e36/lib/jsonapi/serializable/relationship/dsl.rb#L15-L29

Not sure how to handle it without code duplication.

[smaximov]

@beauby, how about I move fetching serializable class to a separate class/module, so we can get meaningful errors without code duplication?

[beauby]

@smaximov Yeah, makes sense. It was all within a ResourceBuilder class before, but it had too complex an API. If you have a good name/architecture in mind, go ahead! :+1:

[beauby]

I think for now we should go with a JSONAPI::Serializable.class_for(object, hash) module function.

[smaximov]

I think for now we should go with a JSONAPI::Serializable.class_for(object, hash) module function.

Implemented it in 0650192 and don't like it, reasons are boilerplate and duplication:

• As for now, there is no reason to have a helper method to return a serializable class (instead of a resource) because we always instantiate this class right away: 1, 2, 3.

• And some more duplication: 1, 2.

It was all within a ResourceBuilder class before, but it had too complex an API

I think having a separate class for this is a good idea. Aside from addressing the issues mentioned above, it helps testing resource building logic in isolation.

[beauby]

Hi @smaximov – thanks for looking into this. The one thing I do not like about this ResourceBuilder pattern is that it creates a throw-away object for every relationship, hence my suggestion of a module function (previously, an instance of ResourceBuilder was passed around within exposures, but I don't really like that pattern either because it prevents calling as_jsonapi directly on a resource without knowledge of the internals). How about having a JSONAPI::Serializable.resource_for(object, inferrer) then, which would instantiate the resource class? It would leave a tiny bit of duplication (namely the if blah.respond_to?(:to_ary); ... ; else; ...; end, but I think that's ok).

[smaximov]

I agree with you that creating an instance of J::S::ResourceBuilder doesn't make much sense and it's not very good performance-wise.

How about having a JSONAPI::Serializable.resource_for(object, inferrer) then, which would instantiate the resource class?

Why not make J::S::ResourceBuilder a module then?

It would leave a tiny bit of duplication (namely the if blah.respond_to?(:to_ary); ... ; else; ...; end, but I think that's ok).

I think it's easy to work around it.

[beauby]

Why not make J::S::ResourceBuilder a module then?

It's debatable, but I personally do not like modules/classes with only class methods. Moreover, I think building a resource/resources is central enough to the functionality of this gem that it makes sense to have those two methods on the main module, i.e.:

• JSONAPI::Serializable.resource_for(...)
• JSONAPI::Serializable.resources_for(...)

What do you think?

[smaximov]

Personally I like having a separate module more (because it represents single concern), but I'm fine with your suggestion. So I'm moving resource building methods to the main module?

[beauby]

@smaximov Yes please 🎉

[beauby]

LGTM, merging. Thanks for your work @smaximov!

[smaximov]

You're welcome :)

[WaKeMaTTa]

@smaximov Thank you so much for this PR. You saved my day.

[stevenharman]

@beauby Any chance of cutting a new release to get this out? I just tripped over the same thing.