Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support compound_fields custom ecto type #517

Open
stanleygtrillion opened this issue Nov 19, 2024 · 6 comments
Open

Support compound_fields custom ecto type #517

stanleygtrillion opened this issue Nov 19, 2024 · 6 comments

Comments

@stanleygtrillion
Copy link

currently compound_fields automatically hardcode type to :string
but in the case of compound_fields of 2 uuid fields, performing filter to this compound_field will fail

e.g.

compound_fields: [
  new_uuid: [
    :uuid1,
    :uuid2,
  ]
],

with op like_or will generate uuid1 like $1 OR uuid2 like $1
this will fail because uuid operation with like throw error at postgres
e.g: select * from my_table where id like '<uuid value>';

is it possible to defined the ecto_type on compound_fields so on building the like_or operation it will generate uuid1 = $1 OR uuid2 = $2

@woylie
Copy link
Owner

woylie commented Nov 19, 2024

I think you'll want to use the :in operator here and pass the list of UUIDs you want to filter on as filter value.

@stanleygtrillion
Copy link
Author

stanleygtrillion commented Nov 20, 2024

:in will not work in this scenario, it will throw this error:

%{
  "filters" => %{
    "0" => %{
      "field" => "new_uuid",
      "op" => "in",
      "value" => ["8ff7ccfd-b896-41e4-97f6-4b4516f57f62"]
    }
  }
}

Invalid Flop: #Ecto.Changeset<action: :replace, changes: %{first: 25, filters: [#Ecto.Changeset<action: :insert, changes: %{value: [\"8ff7ccfd-b896-41e4-97f6-4b4516f57f62\"], op: :in, field: :new_uuid}, errors: [op: {\"is invalid\", [allowed_operators: [:=~, :like, :not_like, :like_and, :like_or, :ilike, :not_ilike, :ilike_and, :ilike_or, :empty, :not_empty]]}], data: #Flop.Filter<>, valid?: false>], order_by: [:inserted_at], order_directions: [:desc]}, errors: [], data: #Flop<>, valid?: false>

this is because :in is not part of the allowed_operators for compound fields

@stanleygtrillion
Copy link
Author

@woylie mind taking a look at this PR: #518
i have added test cases for the new proposed :or operator to facilitate this operation
do let me know how i can improve it.
thank you

@stanleygtrillion
Copy link
Author

I think you'll want to use the :in operator here and pass the list of UUIDs you want to filter on as filter value.

or maybe we can simply add :in into the allowed operators for compound field? @woylie

@woylie
Copy link
Owner

woylie commented Nov 30, 2024

Having or as an operator leads to awkward semantics, since normally, the operator is applied on the field value and filter value (WHERE field OPERATOR value).

I think what you really want here is to apply an :== operator on a compound field, and for the compound field to be an Ecto.UUID type. Neither is currently supported. Note the filter operator rules for compound fields in the docs, especially the last bullet point about currently unsupported operators, and the proposed application rules for them. Since compound fields were specifically added for enabling search operations on string fields, the filter value is split by white space characters (unless the filter value is given as a list).

It might be possible to allow compound fields for other Ecto types than string. This would involve:

  • Add an ecto_type option to the compound field config (default :string).
  • Set the operators of the FieldInfo struct for compound fields depending on the Ecto type.
  • Only split the filter value if the Ecto type is :string.
  • Update the filter operator rules for compound fields that are not string fields.

I'm not quite convinced that this is a good idea, though. This would increase the complexity of compound fields and might easily lead to behaviour that is unexpected for library users.

In your example, you are applying a like operator on a UUID column. This wouldn't work even with the changes, since you would need to cast the UUID column as a string for that to work. I think the best solution for you would be to define a custom field that applies the casting and application on the two columns.

@stanleygtrillion
Copy link
Author

stanleygtrillion commented Dec 6, 2024

@woylie actually im not applying like operator on the uuid column. you can check the PR #518 and i have added a test for this as well

yes essentially i want to do :== operator on the compound fields
im doing quite a complex compound field for my need

so what im proposing is this :or operator will use :== op
and since for :== operator usage, it doesnt make much sense to split the filter value
just apply direct :== op on the value: https://github.com/woylie/flop/pull/518/files#diff-14c968bdfea3fb2a9443642581c43ad454db8b13c6e57f32cbec147d3f4ce73cR543

there is just some field that cannot accept like operator
and the existing ilike_and, ilike_or, like_or, like_and operators already suffice for string compound field

the reason why we cannot use the custom fields is because
we are using columns from joined tables for the compound field

this is the simplified version of what the query is like

schema "transfer_snapshots" do
  ...
  has_one :express_snapshot, ExpressSnapshot
  ...
end

@derive {
  Flop.Schema,
  filterable: [
    :digital_asset_id,
  ],
  adapter_opts: [
    join_fields: [
      express_from_digital_asset_id: [
        binding: :from_wallet_asset,
        field: :digital_asset_id,
        ecto_type: :string,
        path: [:express_snapshot, :route, :from_wallet_asset]
      ],
      express_to_digital_asset_id: [
        binding: :from_wallet_asset,
        field: :digital_asset_id,
        ecto_type: :string,
        path: [:express_snapshot, :route, :to_wallet_asset]
      ],
    ],
    compound_fields: [
      digital_asset_id: [
        :express_from_digital_asset_id,
        :express_to_digital_asset_id,
      ]
    ]
  ]
}

TransferSnapshot
|> join(:left, [t], e in assoc(t, :express_snapshot), as: :express_snapshot)
|> join(:left, [express_snapshot: e], r in assoc(e, :route), as: :route)
|> join(:left, [route: r], fwa in assoc(r, :from_wallet_asset), as: :from_wallet_asset)
|> join(:left, [route: r], twa in assoc(r, :to_wallet_asset), as: :to_wallet_asset)
|> preload([express_snapshot: e, route: r, from_wallet_asset: fwa, to_wallet_asset: twa],
  express_snapshot: {e, route: {r, from_wallet_asset: fwa, to_wallet_asset: twa}}
)
|> Treasury.Flop.validate_and_run(params, for: TransferSnapshot)

perhaps there is an easier way to do this without using compound field, we would definitely be glad to explore

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants