Refactor initialization

[Blacksmoke16]

Preface: This is still a WIP and are a few things left to do. However I wanted to make the draft to start to discuss how we expect this to ultimately work.

Changes

• Removes *::Serializable
  • People can just include it/whatever serialization lib they want to use (as long as it works with properties)
• Removes Granite::Type.convert_type
  • Granite no longer handles converting strings/etc to their proper type
• Removes set_attributes
  • Changes implementation to use overloaded #initialize methods
  • Updates #create and #update to use them
  • No longer accepts Hash

Initialization/ivars

The big questions I have are:

1. Should we support Model.new hash?
2. Should we define properties to be nilable under the hood but hidden via property!?
3. Should we just stop trying to define #initialize for the user in first place?

From what I did so far, these are my findings (not diving in to deep).

Supporting 1:

• Moves compile time errors to be runtime errors
  • Such as passing String when column is typed as a Bool
  • Hashes come with a Union of types that tuples and namedTuples do not which has to be dealt with

Supporting 2:

• Allows compile errors when trying to set a not nilable column to nil
  • Current implementation uses nil or the column's default if defined
• Would make it so partial hydration of a model is not possible
  • I.e. would have to return a namedTuple/hash if you didn't want ALL the columns
• Would make it harder to work with deserializing from JSON
  • Since a value would have to be provided for every column, even if some are set internally. I.e. think of some being readonly, not able to be set by the client.

Supporting 3:

• Would remove the need for any of this and let the user define initializers that work exactly how they want them to.
  • Would have more control/compile time errors since properties would be typed more accurately
  • Would remove some magic since it would require an initializer just like any other Crystal class

Once we figure this out I'll update tests to make sure everything works as expected for the chosen implementation.

[robacarp]

I like the idea of empowering the developer with the ability to override the functionality of the initalizer, but I also to boilerplate to a minimum. My priorities go something like this:

1. provide compile time errors where possible,
2. avoid surprising the developer, (eg. by calling the init method when they’re not expecting it)
3. “just work” out of the box without relying on generating a bunch of boilerplate, and
4. Empower the developer to make changes where they want to extend the functionality without requiring them to reinvent the wheel

This seems like it might be closely related with the idea that we could remove the column macro and replace it with a columns macro.

[Blacksmoke16]

This seems like it might be closely related with the idea that we could remove the column macro and replace it with a columns macro.

How would this help? Personally I'd rather see the column macro replaced with the user defining properties/annotations themselves.

@[Granite::Column(auto: true, primary: true, nilable: true)]
getter id : Int64?

@[Granite::Column(nilable: true)]
property year : Int32?

@[Granite::Column]
property! name : String

One thought I had was if we don't define an initializer, and make the ivars nilable behind the scenes, then you could still do model = User.new then set your values. But if you wanted to do User.new name: "Jim", age: 19 you would need to define that initializer in your class (just like any other Crystal class).

I suppose even if we still define our built in initializers, they could still define their own if they wanted more custom behavior?

I'm conflicted on how to balance everything :/

[drujensen]

@robacarp @Blacksmoke16 My take is that the user will not expect (Roberts' #2) an initializer to be created for me. I would not expect a not_nilable property to be nilable, so I would expect that I would need to create an initializer to set that value when I perform a new.

To me, the column macro inherits from the property macro but adds ORM mapping for the save method. Setting the properties should be the same as you do in Crystal.

I vote we remove some of the magic. maybe we can remove the create and update methods as well and have the user only perform a save. If they want to bulk update properties, they can define their own update method. WDYT?

Also, adding to_params and from_params with an include ParamsSerializer similar to to_json and from_json with an include JsonSerializer makes the API clean and easy to grok.

[Blacksmoke16]

My take is that the user will not expect (Roberts' #2) an initializer to be created for me.

Right, if we go that way the only initializer that will be defined by default is the argless one that comes with Class. E.x.

abstract class Base
end

class User < Base
  property name : String?
end

User.new

But if you want to initialize @name you would have to define your own.

class User < Base
  property name : String?
  
  def initialize(@name : String); end
end

User.new "Fred"

Obviously if you add in *::Serializable you could still do User.from_* but directly newing up a model would require you to define an initializer, or use the argless one and just do .property = xx for each one.

I would not expect a not_nilable property to be nilable

I think there might be some misunderstanding on our viewpoints of this. My thought is that internally all properties should be nilable, not the getter/setter for those properties. The current implementation of using property! for the not nilable columns handles this for us. E.x.

column name : String gets expanded to property! name : String with the annotation applied of course.

Using this class:

class User
  property! name : String
end

# Are still able to new up a model
user = User.new # => @name=nil

# Trying to access the value at this point would raise an exception
# After https://github.com/crystal-lang/crystal/pull/8296 is merged it would be like
user.name # => Unhandled exception: User#name cannot be nil (NilAssertionError)

# The default getter removes `Nil` from the union so you won't run into like
# `undefined method '+' for Nil (compile-time type is (String | Nil))`
user.name = "Fred"
typeof(user.name) # => String

# A nilable query method is also added
typeof(user.name?) # => String?

# `Nil` is removed from the setter types so this won't compile
#
# Error: no overload matches 'User#name=' with type Nil
#
# Overloads are:
#  - User#name=(name : String)
user.name = nil

IMO this gives us more options internally while still giving good protection to the end user; such as partial hydration, allowing some properties to be set later (that would be not null in the DB) instead of requiring everything up front. An example of when this would be useful would be: Imagine a JSON api where a user can create a blog post with a body like:

{
  "title": "Title of blog",
  "published": true,
  ...
}

Every post was created by a user, so there would also be a column author_id : Int32 (or could be a relationship like has_one User) that represents the user that created that post. If every field was not nilable, you would be required to supply it within the POST body so it wouldn't fail on Post.from_json. Having it nilable allows you to consume the body then within your controller do like post.author_id = current_user.id.

Also, adding to_params and from_params with an include ParamsSerializer

I'm assuming this would handle HTTP::Params? IMO this is one of the benefits the switch to annotations brings, modules that just do stuff to properties just work 💯.

[drujensen]

@Blacksmoke16 thanks for the details.

column name : String gets expanded to property! name : String with the annotation applied of course.

would it make more sense to use column! name : String so that users know what to expect when the macro expands?

[Blacksmoke16]

I don't think so. Documentation would probably do the trick. I'm not sure what benefit it would bring, the type of the column should be enough indication of what it will do.

[Blacksmoke16]

This wouldn't compile anymore assuming the user doesn't include id within the initializer (which wouldn't make sense since its auto increment anyway).

[Blacksmoke16]

This wouldn't compile anymore since the initializer would be expecting an Int64? not String. If the user wanted they could accept both and set articleid within the initialize method; converting the input value if needed.

[Blacksmoke16]

Won't be able to do this anymore by default

In spec/granite/transactions/save_spec.cr:49:13

 49 | parent2.id = parent1.id
              ^-
Error: undefined method 'id=' for Parent

[Blacksmoke16]

We could probably support #update if we wanted.

1. Call #initialize with the values
2. Support only named arguments and do a loop with a case statement to set the ivar when there is a match

[Blacksmoke16]

@robacarp @drujensen I updated the code to support no default initializers, as that seems to be the direction we're learning towards. I also left some comments on test cases that I removed with the reasoning behind why they were removed.

So far from there I would suggest checking out this branch on some small local projects and see how things work. From there we'll still need to:

1. Include an overload that supports HTTP::Params
2. Decide if we want to support #update
3. Update the docs
4. Anything else I'm missing?

[Blacksmoke16]

@robacarp @drujensen One other thing I thought of. Currently we require the PK to be nilable since it wouldn't have one until it was saved. However that makes things like

user = User.first!
user.id + 1

fail since #id is nilable. What are your thoughts on how to handle this?

I'm thinking the majority of cases are going to be working with objects that originate from a DB query, thus having a PK. So:

1. Keep it the way it is, requiring the user to handle nil as a possible PK value everywhere
2. Switch to requiring the PK be not-nilable and use getter!, in order to keep it nilable but hide that from the user. Would still allowing checking for a PK value using like #id?.
3. Remove the type requirement on the PK and just define either a getter or getter! depending on if the type they give it is nilable or not-nilable.

I'm thinking 2.

[robacarp]

@Blacksmoke16 hiding a nilable field from the implementer feels risky to me.

I think at this point in my Crystal experience, I'd like to see a different object which represents persisted database entries and has a non-nilable primary key and other fields. I digress.

That said, I think the switch you made here is pretty straightforward. Having a nilable PK field is a pain over and over again -- any nilable fields are.

[Blacksmoke16]

hiding a nilable field from the implementer feels risky to me.

I should probably stop saying hide, it's more like just changing the default behavior to be not nilable. But, I see your point. I think I'll change my commit there to require the PK be nilable, but just use getter!. Then its more clear a model might not have a PK until its saved, but help with the more common use cases of accessing it.

I'd like to see a different object which represents persisted database entries and has a non-nilable primary key and other fields

A whole separate object isn't ideal IMO. What would that get us that getter! or property! don't?
If the user does model.id, most of the time i would say is they got it from a db query and it's going to exist. getter! makes the use case the default, while still allowing for a nilable getter, via #id?.

[robacarp]

Well, the point of using a non-nilable field is that you get compile time checks against it. And for situations where the field is actually nil able, an unsaved model, you get compile time enforcement that the field’s nil-ability must be handled. getter!’s nil check is tunable code, which throws an exception, which should be handled somewhere.

With the getter! implementation, when someone makes a mistake and the server goes kaput, how do we lead them down the right path?

[Blacksmoke16]

With the getter! implementation, when someone makes a mistake and the server goes kaput, how do we lead them down the right path?

My thinking is that this is not much different than before where we had model.column and model.column!. We would have documentation that says if you declare a type as non-nilable, then it will be implemented with property! If there is a possibility that a non-nilable column is nil, such as that column wasn't selected, or hasn't been saved yet etc, then they should use model.column?.

This implementation provides the best middleground of protection and useability. As i mentioned in #370 (comment)

[drujensen]

@Blacksmoke16 @robacarp Where did we end up with this PR? This is almost a year old and not sure what we decided.

[Blacksmoke16]

@drujensen IIRC, we never came to a consensus if this is something we wanted to move forward with; given it's a breaking change and changes the semantics quite a bit.