Two Field comparison show some unknow error

ycv005 · May 19, 2019, 3:14pm

Following is my schema

    field :minsalary, :decimal
    field :maxsalary, :decimal

And when I am comparing/validating them using the following code,

    |> validate_number(:maxsalary, greater_than: :minsalary, message: "Max. Salary should be greater than Min. Salary")

I am getting this unknown error. Your response/help will be highly appreciable.

NobbZ · May 19, 2019, 3:31pm

:greater_than requires a value, not a field.

So if you want to compare to another field from the same struct you need to fetch it first.

Still, AFAIR, :greater_than also uses >/2, which will probably not do what you expect here.

ycv005 · May 19, 2019, 4:35pm

I also look into this post and tried same but getting error. Here is my code with error (if you need more info let me know)
Code-

def changeset(job_opening, attrs) do
    job_opening = %{data: %{minsalary: minsalary}} = job_opening
    |> validate_number(:maxsalary, greater_than: minsalary, message: "Max. Salary should be greater than Min. Salary")

Error-

undefined function minsalary/0

LostKobrakai · May 19, 2019, 4:47pm

The pattern match only happens after validate_number has been evaluated. So minsalery is indeed undefined when you use it.

gregvaughn · May 19, 2019, 4:52pm

I assume job_opening is a Changeset struct. Firstly you really shouldn’t go digging into the internals when there’s a provided accessor function. I would write this as:

minsalary = get_field(job_opening, :minsalary)
validate_number(job_opening, :maxsalary, greater_than: minsalary, message: "..."

ycv005 · May 19, 2019, 4:54pm

get_field seems to be user-defined function, right?

gregvaughn · May 19, 2019, 4:55pm

No. I said a “provided accessor”. See https://hexdocs.pm/ecto/Ecto.Changeset.html#get_field/3

ycv005 · May 19, 2019, 4:56pm

Oh…thanks. I was not aware of that. Thanks

ycv005 · May 19, 2019, 5:10pm

Treid get_field

  def changeset(job_opening, attrs) do
    job_opening
    |> validate_number(:minsalary, greater_than: 0, message: "Min. Salary should be greater than 0")
    minsalary = get_field(job_opening, :minsalary)
    |> validate_number(job_opening, :maxsalary, greater_than: minsalary, message: "Max. Salary should be greater than Min. Salary")
  end

but getting Error that is undefined function minsalary/0
When I comment out these error-prone lines and inspect the job_opening, here is what I get

%Apollo.JobOpening.Schema.JobOpening{
  __meta__: #Ecto.Schema.Metadata<:loaded, "public", "job_openings">,
  applicants: #Ecto.Association.NotLoaded<association :applicants is not loaded>,
  applied_openings: #Ecto.Association.NotLoaded<association :applied_openings is not loaded>,
  company_admin: #Ecto.Association.NotLoaded<association :company_admin is not loaded>,
  company_admin_id: 1,
  description: "<p><strong>Join [Company] as a [Position]!</strong></p><p>We're looking for an experienced [Position] to come in and help us take our team to the next level.</p><p>To apply, take this <strong>[TimeLimit] minute test</strong>. All questions can be <strong>answered multiple times</strong>. And you will get rapid <strong>feedback</strong> after finishing.</p><p>You can take the test <strong>only once</strong>.</p>",
  duration: 900,
  employment_type: "Full Time",
  failure_message: "<p><strong>Thanks! You scored [TestScore]!</strong></p><p>No worries. A real human at [Company]. will now go through everything you sent. If you’re chosen for the next round, we'll share the good news by [7DaysFromApplying].</p><p>Talk to you soon!</p><p>[Admin] from [Company]</p>",
  id: 7,
  inserted_at: ~N[2019-05-19 17:08:32],
  is_public: false,
  job_stages: #Ecto.Association.NotLoaded<association :job_stages is not loaded>,
  maxsalary: #Decimal<10>,
  minsalary: #Decimal<10000>,
  name: "Machine Learning Engineer II",
  questions: [],
  share_url_route: "i2eJPs6hTM4ErdNCUbhB",
  skills: ["Deep Learning", "Data analysis", "python", "Data structure"],
  success_message: "<p><strong>Great start! You got [TestScore]!</strong></p><p>A real human will now go through everything you sent. If you’re chosen for the next round, we'll share the good news soon. If the journey stops here, we’ll also confirm that.</p><p>Talk to you soon!</p><p>[Admin] from [Company]</p>",
  test_required: true,
  threshold: 50.0,
  updated_at: ~N[2019-05-19 17:08:32],
  workspace: #Ecto.Association.NotLoaded<association :workspace is not loaded>,
  workspace_id: 1
}

gregvaughn · May 19, 2019, 6:57pm

You are passing the schema struct to the Changeset functions that expect a changeset as the first argument. Try

def changeset(job_opening, attrs) do
    job_opening
    |> cast(attrs, @allowed_fields)
    |> validate_number(:minsalary, greater_than: 0, message: "Min. Salary should be greater than 0")
    minsalary = get_field(job_opening, :minsalary)
    |> validate_number(job_opening, :maxsalary, greater_than: minsalary, message: "Max. Salary should be greater than Min. Salary")
  end

Note: you will need to also define the @allowed_fields attribute or an inline list of allowed field names.

NobbZ · May 19, 2019, 7:08pm

ycv005:

Treid get_field

  def changeset(job_opening, attrs) do
    job_opening
    |> validate_number(:minsalary, greater_than: 0, message: "Min. Salary should be greater than 0")
    minsalary = get_field(job_opening, :minsalary)
    |> validate_number(job_opening, :maxsalary, greater_than: minsalary, message: "Max. Salary should be greater than Min. Salary")
  end

Let me reindent/reformat this piece of code, such that you see more clearly how the compiler sees it:

  def changeset(job_opening, attrs) do
    job_opening
    |> validate_number(:minsalary, greater_than: 0, message: "Min. Salary should be greater than 0")

    minsalary = job_opening
      |> get_field( :minsalary)
      |> validate_number(job_opening, :maxsalary, greater_than: minsalary, message: "Max. Salary should be greater than Min. Salary")
  end

When you want to compare dependent values, I’d first extract them and then pipe all validations one after the other, clearly separating concerns this way:

def changeset(job_opening, attrs) do
  minsalary = get_field(job_opening, :minsalary)

  job_opening
  |> validate_number(:minsalary, greater_than: 0, message: "Min. Salary should be greater than 0")
  |> validate_number(:maxsalary, greater_than: minsalary, message: "Max. Salary should be greater than Min. Salary")
end

But still I have to repeat my warning. validate_number with :greater_than does compare using >/2 as you can see here:

github.com

elixir-ecto/ecto/blob/v3.1.4/lib/ecto/changeset.ex#L302-L309


      
          @number_validators %{
            less_than:                {&</2,  "must be less than %{number}"},
            greater_than:             {&>/2,  "must be greater than %{number}"},
            less_than_or_equal_to:    {&<=/2, "must be less than or equal to %{number}"},
            greater_than_or_equal_to: {&>=/2, "must be greater than or equal to %{number}"},
            equal_to:                 {&==/2, "must be equal to %{number}"},
            not_equal_to:             {&!=/2, "must be not equal to %{number}"},
          }

But you can not properly compare Decimals using >/2 and friends. Thats why there are Decimal.cmp/2 and Decimal.compare/2.

peerreynders · May 19, 2019, 7:21pm

Look further

github.com

elixir-ecto/ecto/blob/v3.1.4/lib/ecto/changeset.ex#L2032-L2052


      
          defp validate_number(field, %Decimal{} = value, message, spec_key, _spec_function, target_value) do
            result = Decimal.cmp(value, decimal_new(target_value))
            case decimal_compare(result, spec_key) do
              true  -> nil
              false -> [{field, {message, validation: :number, kind: spec_key, number: target_value}}]
            end
          end
          
          defp validate_number(field, value, message, spec_key, spec_function, target_value) do
            case apply(spec_function, [value, target_value]) do
              true  -> nil
              false -> [{field, {message, validation: :number, kind: spec_key, number: target_value}}]
            end
          end
          
          defp decimal_new(term) when is_float(term), do: Decimal.from_float(term)
          defp decimal_new(term), do: Decimal.new(term)
          
          defp decimal_compare(:lt, spec), do: spec in [:less_than, :less_than_or_equal_to, :not_equal_to]
          defp decimal_compare(:gt, spec), do: spec in [:greater_than, :greater_than_or_equal_to, :not_equal_to]
          defp decimal_compare(:eq, spec), do: spec in [:equal_to, :less_than_or_equal_to, :greater_than_or_equal_to]

Though I was a bit surprised that validate_number supported Decimal directly - but given that it is one of the Ecto primitive types I guess I shouldn’t have been.

ycv005 · May 20, 2019, 12:31pm

Here is my working code on comparing two field

    |> validate_maxsalary()
  end


  def validate_maxsalary(changeset) do
    minsalary = get_field(changeset, :minsalary)
    maxsalary = get_field(changeset, :maxsalary)
    if minsalary > maxsalary do
    changeset |>  add_error(:maxsalary, "Max. Salary should be greater than Min. Salary")
    else
      changeset
    end
  end

Thanks to all for such actions.

NobbZ · May 20, 2019, 12:37pm

This definitely will not work properly because of the reasons already mentioned.

Please stick to validate_number, it knows how to deal with decimals as @peerreynders pointed out. If you want to stick with your custom validator, make sure to use Decimal.cmp/2/Decimal.compare/2.