korma / Korma

Tasty SQL for Clojure.
http://sqlkorma.com
1.48k stars 222 forks source link

Restricting fields in select isn't possible after defining them on the entity #251

Open iartarisi opened 9 years ago

iartarisi commented 9 years ago

I have the following entity defined:

 (defentity games
   (table :games)
   (fields :board :white :black :moves)
   (belongs-to player-w {:fk :white_id})
   (belongs-to player-b {:fk :black_id}))

Now when I try to do a select with just one column I get:

(dry-run (select games (fields :white_id)))
dry run :: SELECT "games"."stones", "games"."white_id", "games"."black_id", "games"."white_id" FROM "games"

IMHO this is a bug, as the expected output is:

SELECT "games"."white_id" FROM "games";
immoh commented 9 years ago

This is how fields works so that you can compose queries. You can easily work around it by not defining unwanted fields in defentity.

iartarisi commented 9 years ago

Well this wasn't the behaviour in 0.3.5. I used to have the fields defined and setting fields in the query would ignore the default fields from defentity.

If this is the new way that this feature should work, then it should be documented.

The current documentation (http://sqlkorma.com/docs#select) describes the previous behavior:

If you do so, you'll also want to specify the exact fields to be returned in the query using the (fields) function, which takes a variable number of keywords representing the field names you want.

Arcanum-XIII commented 9 years ago

I have to agree with @mapleoin on this : this behavior is not documented and so tricked me into the same problem. I don't understand your answer @immoh in fact, maybe I've missed something ?

immoh commented 9 years ago

@mapleoin: I assume you're talking about 0.3.0-RC5.

Looks like back then fields (and entity-fields) didn't set any default fields for the entity:

(defentity games
  (fields :board :white :black :moves))
;;=> (var user/games)
(sql-only (select games))
;;=> "SELECT \"games\".* FROM \"games\""

Behavior of fields on a query was same as it is now (adding fields instead of replacing them):

(-> (select* :games)
    (fields :board)
    (fields :moves)
    select
    sql-only)
;;=> "SELECT \"games\".\"board\", \"games\".\"moves\" FROM \"games\""

I agree that the documentation could more clear about this. PRs welcome :)

iartarisi commented 9 years ago

Right, it was 0.3.0-RC5. Ok, I'll look more into this and try to send a PR.

iartarisi commented 9 years ago

Ok, so I've opened https://github.com/korma/sqlkorma/pull/10 to address this in the documentation. However, after writing that, I am even more convinced that this is a bug because if it's hard to document then it's a bug :).

AFAICS the behaviour of the entity-fields in defentity depends on whether or not the select query contains a fields section. And that behaviour is totally opposite in the two cases: in the first case, entity-fields restricts the columns being returned and in the second case it expands them.

I think the right behavior should be to only use the entity-fields when the select query doesn't have any fields specified. (It might also be worth renaming it to default-fields)

On a related note, @immoh, is it right that using fields inside the defentity call in my code and your comment (assuming Korma 0.3.0-RC5) didn't do absolutely anything? I might have just misunderstood the documentation when I wrote that code :)

immoh commented 9 years ago

Would it help to stop saying "default fields" and document that entity-fields will be included in all select queries using the entity?

I thought the confusion was mostly about fields adding the fields instead of re-setting them. As I demonstrated in my pervious comment, this behavior is the same whether its applied on entity or query (actually entity is just query bound to a var) and hence doesn't really depend on the behavior of entity-fields. Perhaps docstring of fields should be updated to clearly state that it adds fields.

Yeah, based on my quick check in repl It looks like fields inside defentity didn't do anything in 0.3.0-RC5. Probably it was a bug that was fixed later.

iartarisi commented 9 years ago

Would it help to stop saying "default fields" and document that entity-fields will be included in all select queries using the entity?

Right, but I still find that behaviour awkward. Why would you want to have fields defined on the entity included in all queries with no option to override them?

I could see that being useful if we redefined the way we look at entities. If instead of mapping one to one with tables they would just be one view of a table and the documentation would encourage defining many entities on the same table just for the purpose of making some queries easier. But that doesn't seem to be the intention right now.

I thought the confusion was mostly about fields adding the fields instead of re-setting them. As I demonstrated in my pervious comment, this behavior is the same whether its applied on entity or query (actually entity is just query bound to a var) and hence doesn't really depend on the behavior of entity-fields. Perhaps docstring of fields should be updated to clearly state that it adds fields.

Right. Perhaps I'm coming at this with the wrong mindset (python/ruby ORMs), but yes, I would expect that if only one set of fields is specified in a query, then only those columns are returned. Returning extra columns which were defined somewhere else is a bit of a magic behavior (unless fields was renamed to extra-fields or add-fields which is another can of worms). Again, I'm wondering if it is really Korma's intention to re-invent the ORM.

immoh commented 9 years ago

In Korma, all query manipulation functions add on top of existing query instead of overriding existing values. I'm sorry to hear that you find this behavior magic or unintuitive but I don't think the behavior is going to change in near future.

dvcrn commented 9 years ago

I am running into the exact same issue. I just recently started implementing korma into my project and don't see the point as well why there is no option available to override fields or limit behavior.

The way I see it, it is supposed to work like this: (select user) -> Results in SELECT * (select user (fields :user_id)) -> Results in SELECT user_id

Taking a look at different database mappers, for instance in django: Entities define how a entity is displayed in the database, including all it's fields and relations it has to other entities. These fields are more a summary of what the entity consists of rather than a list of default values. Just because I have a password field defined in my user entity doesn't mean I want to have the users password in every query.

Having entity-fields and fields as "output fields" is confusing and doubled functionality. Also if I remove the entity-fields from my model, developers no longer know from looking at the entity code what fields are available in a table. They have to either open a database shell and check it, look into the migrations or guess.

I'm giving my +1 to changing this functionality back the way it makes more sense. Change the current behavior to default-fields rather than entity-fields and let (fields) limit the output.

fbauer commented 9 years ago

I too am surprised by this behavior. Reading the documentation, I assumed that entity-fields would merely be the default value which could be overriden by specifying fields within select. With the current implementation, is there a way to restrict the number of columns in the output, given an entity defined by someone else? Or should you always define your own entity in this case?

immoh commented 9 years ago

@fbauer: You shouldn't define them as entity fields if you don't want them in the output.

fbauer commented 9 years ago

@immoh: I changed my code and don't define entity-fields anymore.

Would it help to stop saying "default fields" and document that entity-fields will be included in all select queries using the entity?

That would help. Changing "default fields" to "fields that will be included in all select queries" would have solved the ambiguity for me.

hxzon commented 9 years ago

Can rename entity-fields to default-fields? it will more clear.

rukor commented 7 years ago

Hi,

Does this mean that you cant perform aggregate functions (GROUP, SUM) on tables with entity fields?

paomian commented 6 years ago

I'm agree with https://github.com/korma/Korma/issues/251#issuecomment-56446366, I think this is better behaviour.

venantius commented 6 years ago

I'm going to leave this issue open.

The feature as it currently exists clearly causes confusion, as evidenced by the fact that people keep filing it as a bug (ref. #286 and #340). I personally thought it was a bug when I ran into it about 9 months ago. I also think the way setting fields on an entity currently works is just not an ideal interface. If we're going to argue that it should continue to function this way as a matter of composability then we should provide a mechanism for people to exclude those fields easily when making entity queries.