Primary key methods that are overridden can raise exceptions

[jwoertink]

Take a look at this example: https://play.crystal-lang.org/#/r/a4y0

If you override one of the primary key methods to do your own deal, like in this example, you'll run in to a compile-time error when you wouldn't expect it.

The idea here was basically doing a custom delete where it was expected to basically delete the current model, and delete another model.

In any case, the fix for this is to add include Avram::PrimaryKeyMethods to the Base model, and things are fine. I don't think this is something we need to patch because it's sort of an edge case that you'd define your own version of one of these methods when they're already included; however, I think we can work out the error message a little nicer to maybe give the user a heads up that they can include this module if they need.

[robacarp]

Is the suggestion then to instead do something with a before/after hook?

[jwoertink]

Well, personally, I'd recommend just not overriding methods like delete, but if you need to, then the suggestion is to just include the Avram::PrimaryKeyMethods module in your model class that overwrote the delete method.

My guess is the next release we're going to start working on will actually solve the original issue of why it had to be overwritten in the first place, and this will all be moot.

[matthewmcgarvey]

Just using this as a place to put my thoughts...

The way it is currently set up, Avram::Model has all the methods defined that would normally require a primary key but they raise a compile time error, instead. In user defined models, when primary_key is used, then it includes the Avram::PrimaryKeyMethods module that actually defines those methods.

It's set up in this way so that Avram::Model always has those methods available and classes that take in and call those methods don't have to have special type signatures.

The failure of this setup is that people might want to override and implement special logic around the methods that were moved into the module and then call super to get the original functionality. This is totally understandable and was not considered when the changes were made.

Potential Solution

I'm thinking that we might go down the route that Rails has for creating read-only models where it is a configuration switch in the model. Instead of only including the features when they are needed in the model, we take them away when they are not used. How this looks in Avram, I'm not sure. There's two pieces that would need to have separate fixes:

• the primary-key being present/missing
• the db table being read-only