Sequel orm support

[Aryk]

Following up on the ticket I originally created to make this library truly ORM agnostic (see: #1006).

I got alot of it working, laid the foundations, reorganized the files, and set this up to be a multi-orm library, but jsonapi-resources has a ton of functionality and I ended up spending a lot of time with the last 20% of functionality. I'm also not sure of the maintainers appetite to merge a significant refactoring such as this, hence putting a lot of my time at risk.

Here are some more details:

• Decoupled library from ORM ActiveRecord fully. You don’t need ActiveRecord required for specs or library to work.
• Tests can be run with any ORM via simple environment flag (defaults to ActiveRecord)
• Test fixtures and schemas are kept in ActiveRecord format and outputted to *.sql files.
  • This ensures that we have consistent specs across any ORM and don’t need to reimplement fixtures and the schema for any new ORMs added.
• Fix bug where if you run one spec that calls assert_query_count before a model is instantiated, it will fail because the ORM may run startup sql statements to check column names and primary keys.
• Renamed ActiveRecordAccessor to ActiveRecordRecordAccessor. It is redundant, but its confusing when you don’t have the extra Record. Is ActiveRecordAccessor, supposed to be “ActiveRecord” “Accessor" or “Active” “RecordAcccessor”, it’s really ActiveRecord AND the RecordAccessor. That way, when we add SequelRecordAccessor or OrmRecordAccessor, everything is consistent and works.
• Moved common methods (non-orm specific) up into the RecordAccessor so that subclasses can overwrite (or not) for their specific functionality.
• Remove ActiveRecord specific calls out of resource.rb and other orm-agnostic files.

Some Outstanding Questions/Thoughts:

• A lot of library files use this “method_name” convention. Is that to avoid conflicts in case the user overwrites? Are these essentially protected or private methods? I feel like this convention is only going half way right now, which is very confusing. Sometimes I see methods that seem like they should begin with “” per your convention, but they do not. Is the logic that only methods that we would expect the user to overwrite should not begin with underscore?
• Sometimes you call something “foo_class” and sometimes “foo_klass”. What convention do you guys stick to? Obviously if it is just “class” then you need to use “klass”, but other than that, why not just call things foo_class instead of foo_klass?
• The "RecordAccessor#records_for_relationship” has some strange assumptions. #records_for can in theory return a belongs_to association or simply just an object, yet if there were filters and such passed in, things would blow up because it is not a relation. However this condition doesn’t get hit because those filtering and sorting options are not passed in. It seems like we are kind of overloading this function and it is hard to follow because, for example, #records_for is not returning what you think it is returning.

There are some more refactorings I feel should be done:

1. Why does the RecordAccessor depend on the reference_class, and not the actual record. Seems like it should be ActiveRecordRecordAccessor.new(ar_model_instance) and not how it currently is. I put the methods that operate on the model on the class level methods of the recordaccessor, but I think they should be on the instance level.
2. ActiveRecord has a restrict_with_exception option on associations. Sequel does not. Ideally the person working on this, could add that into the AssociationDependencies plugin. Jeremy will likely merge it. Otherwise, you won't be able to get all the specs working.

Anyways, I'm leaving this PR up, perhaps someone wants to take it across the finish line. 😄 I will not be continuing work on this.

If you do work on this and have questions, feel free to ask, but try to bundle your questions so I can answer more efficiently. Thanks!

[lgebhardt]

@Aryk This a quite a major set of changes. As I see it quickly looking through you are changing major components of the project. It looks like this PR covers:

• Refactoring ActiveRecordAccessor to ActiveRecordRecordAccessor. We thought about this when naming ActiveRecordAccessor, but ultimately decided on the current name. I could be convinced on a rename if it makes the code clearer.

• You moved a lot of functionality back into RecordAccessor. I don't understand why. It seems to defeat what we were going for with the RecordAccessor concept.

• Refactoring of the tests. I haven't looked at every change here, but they look like positive changes to the project in general. Some I've been wanting to make, but haven't taken the time.

I think it would be better to address these as separate PRs. That would minimize the conflicts as we work on new versions of the SequelRecordAccessor. It will also make review a manageable task.

Thanks for taking this on.

[Aryk]

@lgebhardt

Thanks for the feedback:

Refactoring ActiveRecordAccessor to ActiveRecordRecordAccessor. We thought about this when naming ActiveRecordAccessor, but ultimately decided on the current name. I could be convinced on a rename if it makes the code clearer.

Well, I think the think that solidifies the decision, is lets say you were doing meta programming, and had "#{self.orm_type.to_s.classify}RecordAccessor", it won't work with :active_record unless you called it :active, so its consistency in naming always wins and avoids confusion even if the name is awkward. 😄

You moved a lot of functionality back into RecordAccessor. I don't understand why. It seems to defeat what we were going for with the RecordAccessor concept.

Which functionality specifically? I really hope I did not add more reference_class stuff into record accessor. I think I wrote in the original ticket but I really feel things should be refactored to record accessors can instantiated with the orm model and have no jsonapi-resource specific code in them. Please comment specifically where I added functionality that is not related to accessing the record on the record accessor.

Refactoring of the tests. I haven't looked at every change here, but they look like positive changes to the project in general. Some I've been wanting to make, but haven't taken the time.

Yeah, this was a big pain, especially when it came to fixtures and schema, but my solution I think is good especially to make adding other orms easier.

[lgebhardt]

Which functionality specifically? I really hope I did not add more reference_class stuff into record accessor. I think I wrote in the original ticket but I really feel things should be refactored to record accessors can instantiated with the orm model and have no jsonapi-resource specific code in them. Please comment specifically where I added functionality that is not related to accessing the record on the record accessor.

RecordAccessor is meant to be an abstract class. For example by adding the find method as you have RecordAccessor is again tied to the ActiveRecord, which means a PORO or NoSQL RecordAccessor will also be tied to ActiveRecord.

[lgebhardt]

Would you be willing to create a few PRs from this?

• One to rename ActiveRecordAccessor to ActiveRecordRecordAccessor. I agree on the naming, but I'm not sure this is critical for the next two. So I'd consider this optional for now.

• Another to refactor the existing tests

• Finally one to implement the SequelRecordAccessor with tests

In general I think it's best to add one feature, refactor, or bug fix per PR.

[Aryk]

RecordAccessor is meant to be an abstract class. For example by adding the find method as you have RecordAccessor is again tied to the ActiveRecord, which means a PORO or NoSQL RecordAccessor will also be tied to ActiveRecord.

It is an abstract class, it has an interface, that's all it is. I make no reference to "active" anything. Yes there is a find method. That is part of the interface for RecordAccessor. You could rename "find" to "get", but "find" simply gets the record from the orm. This gem has to have some basic requirements about the RecordAccessor and that is one of them. The fact it has the same name as the ActiveRecord#find is simply because "find" isn't a bad name. I was going to call the method "i_summon_an_orm_model_record", which then calls Model.find on the ActiveRecordRecordAccessor implementation, but thought it was a little long of a method name, so I kept find. 😉

Would you be willing to create a few PRs from this?

The ActiveRecordAccessor rename is small. I think its a rename in like 3 places, so doesnt really make sense to split out, IMO.

Another to refactor the existing tests
Finally one to implement the SequelRecordAccessor with tests

I 100% agree it's best to split up when you can. It would be very difficult to split these although it sounds nice, but you need to implement the Sequel accessor (aka 2nd orm) to see where the refactoring of the existing tests needs to be done. Separating is probably possible is possible, but I would rather somebody takes over this projects and does even bigger refactors while reducing code, separating concerns, and getting all specs passing, rather than focusing on how to best split up PRs. And if they have extra time to split up PRs, then great. IMO, there is a big PR because the codebase has significant flaws when it comes to adding another orm, although the test suite is pretty extensive, so kudos for that! 👍

Like I mentioned early, I've moved on from this and decided for an approach with just serializing my response while I handle JSON-API like calls manually. I'd be happy to revisit this gem in the future if somebody else takes Sequel support across the finish line. :)

My advice would be to rewrite the RecordAccessor:

1. It should not have any dependencies on a Reference object. Instead Reference should call to the RecordAccessor and not back and forth like it is now.
2. Filters, sort, includes and how those are combined should not be called from within RecordAccessor. Instead the instructions should be combined somewhere else and then RecordAccessor should be called with an orm-agnostic interface to apply those instructions, something like:

SequelRecordAccessor.find(instructions)

And then the RecordAccessor decides what to do with those instructions. Right now, there is a lot of logic that is not simply about returning records from an orm (ie cached_resources_for , which actually returns resources, not orm records). That shouldn't be there, it's a record accessor, not a resource accessor. ;)

Anyways, that's my two cents.

[lgebhardt]

This code is ActiveRecord based. The intent of the RecordAccessor was to remove all of this so other ORMs will not be dependent on ActiveRecord. The original RecordAccessor was designed to have only abstract methods and no logic.

[Aryk]

How is it "AR based" exactly. Yes I moved it up into record accessor because I was trying to seperate ORM specific calls like Model.find (in AR) vs Model[] (in sequel) and the instructions from the resource.rb about which record to get. filter_records is actually a perfect example of what I was saying in my previous comment about how RecordAccessor.find should probably take in some kind of Instruction class/object that simply has the sort, filters, and include. So this should be taken out of RecordAccessor entirely include RecordAccessor never needs to simply "filter" records. It shouldn't even be a public api function.

[lgebhardt]

It is an abstract class, it has an interface, that's all it is. I make no reference to "active" anything. Yes there is a find method. That is part of the interface for RecordAccessor. You could rename "find" to "get", but "find" simply gets the record from the orm. This gem has to have some basic requirements about the RecordAccessor and that is one of them. The fact it has the same name as the ActiveRecord#find is simply because "find" isn't a bad name. I was going to call the method "i_summon_an_orm_model_record", which then calls Model.find on the ActiveRecordRecordAccessor implementation, but thought it was a little long of a method name, so I kept find. 😉

I made a comment above on your code change showing where I was referring.

I'll look this over again in the morning and consider your other points.

[lgebhardt]

I think we are trying to solve different things with the RecordAccessor class, and I think the naming of RecordAccessor is the root of the confusion. In hindsight I should have named the RecordAccessor something like ResourceAccessor so we would have ActiveRecordResourceAccessor. The idea being that ResourceAccessors are not meant to be ORMs like ActiveRecord, but rather to provide a common higher level interface between JR and the underlying ORM.

[lgebhardt]

@dgeb and I have been discussing the architecture and we're looking into some changes related to the relationship between Resources, RecordAccessors, and caching that we hope will clean things up a bit.

[Aryk]

But its not just that. I highly recommend that whatever accesses the ORMs do not know about resource and just do their job (access the orm to get records from it). You are going back and forth right now and it's confusing (its also breaking separation of concerns principle). Maybe infact, the orm accessor shouldn't even return records from the orm, but just a simple "data" or "included" array and that is it (idk, i havent really tried, but i would give it a try). That way the jsonapi-resources library simply has an interface with some kind of data provider (like an orm) and that's it. The data provider could in theory be another api if you wanted. To the library itself it wouldn't matter.

[dgeb]

You are going back and forth right now and it's confusing (its also breaking separation of concerns principle).

@Aryk Larry and I both share your concerns - these are the architectural issues we've discussed untangling. There is also some entanglement with caching in the RecordAccessor, which I believe drove the initial implementation. We are looking at alternative implementations to provide a cleaner separation of concerns all around.

[Aryk]

@dgeb Great, I think you will need to refactor a lot, so I would also suggest that anyone who wants to build on what I did wait until you do this refactoring. I think it will be much easier to reason about the app after it will be done.

[mmun]

I'm curious to hear about the alternative factoring you have in mind @dgeb. I think it would be worthwhile to try and accommodate PORO models as well.

Since @Aryk has expressed that he is not able to invest more time into this PR we'll need to cooperate and find a champion for splitting this PR up.

[dgeb]

I'm curious to hear about the alternative factoring you have in mind @dgeb.

@mmun Larry is going to investigate how to best remove caching entirely from the RecordAccessor, which will make it more flexible and focused exclusively on records (not resources).

I think it would be worthwhile to try and accommodate PORO models as well.

I agree, and this should be possible with a leaner, record-only-focused RecordAccessor.

Since @Aryk has expressed that he is not able to invest more time into this PR we'll need to cooperate and find a champion for splitting this PR up.

After @lgebhardt's first pass at the RecordAccessor refactor, splitting up this PR should be much more tractable. I'm sure that help with the Sequel-specific pieces would be especially welcome.

[lgebhardt]

I've closed this since @Aryk indicated he will not have time to rework this PR. I would still like to get Sequel ORM support into the project. After #1045 this should hopefully be a bit easier.