Attributes not coerced when persisted about model HOT 12 CLOSED

IMHO I think should be coerced before and after, maybe if we coerce the value before, after it doesn't make sense and so the correct value coerced is stored in the database (or another system) with the correct type

from model.

The coercion is the job of the Repository. After the repository persist the entity, it does not refresh the entity thus you might see the entity's values are not coerced.

If you look into the DB, the stored values are coerced. And when you use Repository to find the entity, this time the entity values would reflect correctly.

I think it is a good idea to refresh the states of the entity after successful persist operation. Thoughts?

from model.

@joneslee85 I'm actually seeing the age stored as a string in the DB, not an integer.

Created with User.new(name: 'Luca', age: '32'):

{
  "age":  "32" ,
  "id":  "1e1715df-0528-4813-a65c-896d470c2fdd" ,
  "name":  "Luca"
} 

Created with User.new(name: 'Luca', age: 32):

{
  "age": 32 ,
  "id":  "0a50695a-e5d2-4796-b8bd-16ef3864c248" ,
  "name":  "Luca"
}

When the object is serialized before persisting, the following method is called:

# lib/lotus/model/mapping/coercer.rb

instance_eval %{
  def to_record(entity)
    if entity.id
      Hash[#{ @collection.attributes.map{|name,(_,mapped)| ":#{mapped},entity.#{name}"}.join(',') }]
    else
      Hash[#{ @collection.attributes.reject{|name,_| name == @collection.identity }.map{|name,(_,mapped)| ":#{mapped},entity.#{name}"}.join(',') }]
    end
  end
}

It just calls the accessor methods on the object and does no coercion. An SQL database will do the coercion automatically since each column has a type, but JSON stores do not.

from model.

@joneslee85 I agree; the entity should be updated after being persisted. This process could include injecting default values and timestamps (see: #134).

The down side is this requires an additional database call.

from model.

@angeloashmore I'll have further discussion with @jodosha as I try to avoid additional calls. Thanks for reporting.

from model.

@angeloashmore What's your use case? Is that database shared from with other services that need a stricter type check on data?

Until know, #to_record doesn't coerce because that information about the Ruby type can be lost while writing on certain databases.

Think of Redis:

[1] pry(main)> require 'redis'
=> true
[2] pry(main)> r = Redis.new
=> #<Redis client v3.2.0 for redis://127.0.0.1:6379/0>
[3] pry(main)> r.set('x', Integer("1"))
=> "OK"

% redis-cli
127.0.0.1:6379> get x
"1"

We coerced our input from String ("1") to Integer and then persisted it. Later on, if we introspect the value stored by database, it's a string. Our coercion get lost, because the database doesn't support that type.

From a Lotus::Model perspective, it should guarantee that a value fetched from the database will be coerced into the mapped Ruby type.

It's true, we're lucky enough in SQL world, where database engines will anyway coerce that value into an integer because we have types in schema definitions.

On the other hand, I'm not easy to add a computation overhead for the most common scenarios that don't need it.

What to do then?

Option 1
There is an extension point for adapters when they explicitly ask for database conversion. Sql::Collection is an example: it has a @mapped_collection which is an instance of Mapping::Collection. It's an object that holds the mapping definitions for that collection.

When it needs to create a record, it asks to the @mapped_collection to serialize it. But you can do the opposite:

private
def _serialize(entity)
  MyCoercer.new(entity).coerce
end

Option 2
Add another setting in Model::Configuration to specify a coercer object to pass when the mapper is instantiated. @joneslee85 thoughts on this option?

from model.

@angeloashmore There is a different issue about the returned entity from a BookRepository.create operation, and I agree with you: it should return the coerced entity.

book = BookRepository.create(Book.new(price: '10'))
book.price # => "10" WRONG, it should be an integer.

This can be easily fixed.

from model.

@jodosha @joneslee85 In my opinion, attributes should be coerced via the mapper before persisting. Is this what you mean with Option 2?

This would allow the adapter and database library to handle any entity/database type differences.

In your Redis example, the Redis library accepted an Integer despite not handling Integer as a type. The library automatically saved a String instead. Let's say in the future the user changes the adapter, for example to RethinkDB. This new database accepts Integers and thus will keep the type.

Persisting to the database

1. Call Repository#persist
2. Repository coerces attributes using the mapper before persisting.
3. Repository persists the entity to the database with the coerced values.

Retrieving from the database

1. Call Repository#find
2. Repository gets the database record/document and coerces attributes using the mapper.

This ensures the following:

• Adapter recieves all attributes with the correct, expected types
• Adapter or DB library handles any type changes if necessary (e.g. unsupported types)
• Database methods can run queries in an expected way (e.g. we can now assume :age will always be an Integer in the DB, so we can run #sum)
• Retriving using the Repository always returns the correct types

I noticed the issue when running math functions (e.g. #sum, #avg, #max, #min) directly on the database via a Query object.

The repository was persisting Strings and Integers for the same attribute depending on how the entity was initiated, so the database could not compute the equation due to the type mismatch.

  # raises error if type mismatch on `age` field
  query do
    where { |u| u[:age] > 10 }
  end.sum(:age)

If the database can save arbitrary types and values are only coerced upon deserialization, the equation would need to be computed via Ruby after retrieving all values of the attribute. I would imagine this would be slower (just a guess; I have not actually tested the performance).

from model.

@angeloashmore I was wrong, you were right.

I've replied without having the code around, so my thinking was biased by the initial design.
I've put in place coercion before persistence to avoid to hit the database to reload the record.

Can you please give #141 a try and see if it solves your problems? Because this is a blocking issue, once I get your and @joneslee85 go, I'm keen to release a patch version for this.

from model.

@jodosha Sorry for the late reply. Just had a chance to test it out. Everything looks good! 👍

from model.

@angeloashmore No worries, expect a patch release soon.

from model.

@angeloashmore https://rubygems.org/gems/lotus-model/versions/0.2.2

from model.