Float comparision in ecto

Questions / Help
,
1

Hi, I’m working through the PragProg “Programming Elixir LiveView” book and encountered a strange bug when implementing a changeset. The changeset is as follows:

 def lower_price_changeset(product, new_price) do
    old_price = product.unit_price
    product
    |> change(unit_price: new_price)
    |> validate_number(:unit_price, less_than: old_price)
  end

When I run the function using a product struct with the same unit price as new price, a valid schema where no changes are made is generated (see below). If I run the function using a new_price which is greater than product.unit_price, an invalid schema is generated so I assume that my logic is solid.

iex(52)> p
%Pento.Catalog.Product{
  __meta__: #Ecto.Schema.Metadata<:built, "products">,
  id: nil,
  description: nil,
  name: nil,
  sku: nil,
  unit_price: 9.0,
  inserted_at: nil,
  updated_at: nil
}
iex(53)> Pento.Catalog.Product.lower_price_changeset(p, 9.0) 
#Ecto.Changeset<action: nil, changes: %{}, errors: [],
 data: #Pento.Catalog.Product<>, valid?: true>

Am I missing something obvious or is this a bug? In that case, is there a workaround? I know that float comparison isn’t straightforward so perhaps this causes the issue?

2

In the docs for change it says Changed attributes will only be added if the change does not have the same value as the field in the data.

So in this case I don’t think the validation is run because the unit price hasn’t changed.

3

I don’t often use change/2 so I went to the docs and noticed this:

Changed attributes will only be added if the change does not have the same value as the field in the data

9.0 == 9.0 so there is no change. maybe try:

product
|> change() # make a changeset
|> put_change(:unit_price, new_price)
4

Actually put_change also says If the change has the same value as in the changeset data, it is not added to the list of changes.

I’ve checked the source for validate_number and it uses validate_change internally, which also states

It invokes the validator function to perform the validation only if a change for the given field exists and the change value is not nil .

1 Like
5

The best bet would probably be writing your own validator that adds a changeset error if the values match, or then invokes the validate_number(... less_than: old_priced)

6

Thanks for the answers, good to know that this is the intended functionality at least. Do you think it’d be the “correct” solution to implement my own validator or to simply return an empty changeset? I feel that the code would become worse if I were to implement an essentially identical version of a library function but on the other hand it is inefficient to keep the execution going knowing that no changes will be made. I’m not entirely sure how Ecto translates this to the database level but I can imagine that it is handled in an efficient manner and that the performance penalty is not that big.

7

I am not sure what else should Ecto be doing, can you specify your expected behavior?

But if you really really want to flag a field as changed there’s always Ecto.Changeset.force_change.

8

I expected an invalid changeset yo be generated, as it does when I give set the new_price parameter to a number which is greater than old_price. Intuitively it doesn’t make sense to me why the cases new_price > old_price and new_price = old_price would yield different given the validate_number call in my original post.

9

Ah yes, I thought there was also an equal sign there. Did you try with force_change and see if the validator gets tripped then?

10

I really depends on your use case. What I’m assuming is you want the changeset to throw an error when new >= old.

It will throw an error when new > old because change(unit_price: new_price) marks unit_price as changed and therefore validate_number(:unit_price, less_than: old_price) runs against the changed value.

But change won’t mark the attribute as changed if new = old and therefore it won’t ever run validate_number.

This is probably your best bet

because ecto not seeing the change is a feature to prevent unnecessary db writes when the value wouldn’t change, but you can still force it with force_change.

Otherwise you could add a custom error when the prices are equal and let the normal validation take care of the less_than case. (this is what I actually meant by custom validation, sorry for the confusion)

def lower_price_changeset(product, new_price) do
  old_price = product.unit_price
  product
  |> change(unit_price: new_price)
  |> error_on_equal_or_less_than(old_price, new_price)
end

defp error_on_equal_or_less_than(changeset, old_price, old_price) do
  add_error(changeset, :unit_price, "is equal to old price", old_price: old_price)
end

defp error_on_equal_or_less_than(changeset, old_price, _) do
  validate_number(changeset, :unit_price, less_than: old_price)
end

The changeset won’t show the price changed in the changes: key, but it will have it in the error and the changeset won’t be valid, so you can adapt your business and application logic to match on it.

#Ecto.Changeset<action: nil,
 changes: %{},
 errors: [unit_price: {"is equal to old price", [old_price: 9.0]}],
 data: #Pento.Catalog.Product<>,
 valid?: false>
1 Like
11

Oh right, maybe it would have helped if I paid a little more attention. You are 100% correct.

I’d also advise for a custom validator function in this case. It’s no big deal and I don’t know why people are so averse to it; it’s just a pipe of functions as all others in Elixir. But I do get it, some get cold feet because previous frameworks gave them PTSD. Can relate!

Though in this case I’d make a custom validator that replaces Ecto’s validate_number + less_than in one fell swoop, i.e. makes the check and either adds an error or just returns the original changeset.

12

If you mean with validate_change(changeset, field, validator_function) this also wouldn’t work because the validator is only invoked on changed fields.

13

No, I mean our own function that accepts a changeset + args and returns a changeset. Here:

 def lower_price_changeset(product, new_price) do
    old_price = product.unit_price
    product
    |> change(unit_price: new_price)
    |> validate_price(old_price)
  end

  def validate_price(%Changeset{} = changeset, old_price) do
    new_price = get_change(changeset, :unit_price)

    cond do
      new_price > old_price ->
        add_error(changeset, :unit_price, "is bigger than the old price", old_price: old_price)

      new_price == old_price ->
        add_error(changeset, :unit_price, "is equal to the old price", old_price: old_price)

      true ->
        changeset
    end
  end

An even better option would be to merge both change(unit_price: new_price) and the function above into something like maybe_put_new_price. The benefit being slightly more readable code + not having to pull out the new price via get_change.

1 Like
14

Hard agree. But here we’re already presenting 4 different ways to do things, so I can see why some would be averse! The beauty of the functional approach does make it easier to swap things out and see what works best.

15

@dimitarvp Something just came to mind… change may be the wrong function for this use case if we want the most idiomatic way to error for a dynamic test of new < old when new could be == old.

The docs also say about change:

Note the value is directly stored in the changeset with no validation whatsoever. For this reason, this function is meant for working with data internal to the application.

This function is useful for:

  • wrapping a struct inside a changeset
  • directly changing a struct without performing castings nor validations
  • directly bulk-adding changes to a changeset

But as we’ve seen the issue is that the change is not added when the values are the same.

I haven’t gone through the “Programming Phoenix LiveView” book in detail but I assume this is an exercise? All their changeset examples use cast instead of change and this is the most idiomatic approach to solve this problem, with an extra option.

  • :force_changes - a boolean indicating whether to include values that don’t alter the current data in :changes. Defaults to false

The reason to use cast is that if this changeset will be called with params from a form, it will attempt to cast the string value from the client into float, and return a cast error if that fails.

In which case we’d want to pass the params as an attribute to cast

def lower_price_changeset(%Product{unit_price: old_price} = product, params) do
  product
  |> cast(params, [:unit_price], force_changes: true)
  |> validate_number(:unit_price, less_than: old_price)
end

iex> Pento.Catalog.Product.lower_price_changeset(p, %{"unit_price" => "9.0"}) 
#Ecto.Changeset<action: nil,
 changes: %{unit_price: 9.0},
 errors: [
   unit_price: {"must be less than %{number}",
     [validation: :number, kind: :less_than, number: 9.0]}
 ],
 data: #Pento.Catalog.Product<>,
 valid?: false>
2 Likes
16

You made me realize that I have mistaken force_change(...) with cast(..., force_changes: true). That’s why I was confused at the start of the thread that it’s not working.

Thank you!

1 Like
17

The many options for changeset functions have made them feel intractable in exactly this way… ‘what’s the right way to tackle my edge case that’s not spelled out in the docs’. Just wanted to post that I’m very glad I was corrected and that @acke’s original question was talked this far out as this has helped me.

1 Like
18

It certainly does make them feel that way, and I’m also glad that we can discuss such things in these forums. At least the dev team is always open to contributions to improve the docs so I may give this a shot unless someone else gets inspired. If so ping me and we can work on it together :wink: