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

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

Open
iartarisi opened this issue Sep 13, 2014 · 17 comments

Comments

@iartarisi
Copy link
Contributor

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";
iartarisi added a commit to iartarisi/greybear that referenced this issue Sep 13, 2014
- Adapted syntax
- The moves.game_id column got renamed to moves.games_id to match the referenced table name (games)
- Removed some fields declarations because of:
korma/Korma#251
@immoh
Copy link
Member

immoh commented Sep 17, 2014

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
Copy link
Contributor Author

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
Copy link

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
Copy link
Member

immoh commented Sep 22, 2014

@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
Copy link
Contributor Author

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

@iartarisi
Copy link
Contributor Author

Ok, so I've opened korma/sqlkorma#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
Copy link
Member

immoh commented Sep 25, 2014

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
Copy link
Contributor Author

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
Copy link
Member

immoh commented Sep 28, 2014

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
Copy link

dvcrn commented Nov 11, 2014

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
Copy link

fbauer commented Nov 14, 2014

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?

fbauer added a commit to fbauer/boefie-invest that referenced this issue Nov 16, 2014
Entities do not define fields with entitiy-fields anymore. This had
the side effect of not allowing leaving out columns in select queries.

See korma/Korma#251
@immoh
Copy link
Member

immoh commented Nov 27, 2014

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

@fbauer
Copy link

fbauer commented Dec 4, 2014

@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
Copy link

hxzon commented Jan 6, 2015

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

@rukor
Copy link

rukor commented Sep 5, 2016

Hi,

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

@paomian
Copy link

paomian commented Aug 25, 2017

I'm agree with #251 (comment),
I think this is better behaviour.

@venantius
Copy link
Contributor

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.

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

No branches or pull requests

9 participants