ModelLoader Columns objects support in column loader

[jekel]

We need to support column names in ModelLoader also as Column objects, to correctly fill model fields in complex aliased queries.
Now Loader can't handle them properly, because part of model names can overlap, like id, name etc... and loader iterates only over their names when filling values.

[coveralls]

Pull Request Test Coverage Report for Build 1167

• 17 of 18 (94.44%) changed or added relevant lines in 2 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.006%) to 98.263%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
gino/loader.py	11	12	91.67%

Totals
Change from base Build 1154:	0.006%
Covered Lines:	3960
Relevant Lines:	4030

---

💛 - Coveralls

[wwwjfy]

I have another fix for your issue. Please check. #326
In this way, it's flexible to return any column in any alias, and we can avoid corresponding_column.
Also when user passes random Column to the loader, and we can catch it at the earliest stage.

[jekel]

@wwwjfy AliasLoader works only for GINO crud operations on model, aspecially alias method and does not work for standard sqlalchemy query aliased object, right?
Because in my case, i have (Model, aggregated_column) in aliased query

[wwwjfy]

I'll check and make needed changes for any alias, but you get my idea, right? We can use Alias.columns to get the Column, and no need to use corresponding_column on model, which has the same limitation and not very friendly to use

[jekel]

Yeah... i will try to find solution to modify AliasLoader for use on aliased query object
but i think we need ability to use Column object also to load model fields?

[wwwjfy]

Using ColumnLoader should cover the case. Because ModelLoader is tightly coupled with model, so it's not necessary to pass an object can be retrieved from the model.
For aliases not related to models, with the same reason, we can't use ModelLoader, because it returns an object of model class.

Let me think of a valid alias use case and see how can be easily used.

[jekel]

For aliases not related to models, with the same reason, we can't use ModelLoader, because it returns an object of model class.

why not?
it searches for valid columns in raw result row and fills model with theirs values, so there can be passed any query to load model from, right?

[wwwjfy]

I might be not clear. By alias not related to models, I mean like (select 1, 2, now()) a. This temp table/alias a is not related to model, thus we don't have a model class for ModelLoader to fill.

If I understand correctly, you want to be able to use both ModelLoader(User, 'id', 'name' and ModelLoader(User, User.id, User.name). My argument is the former one is enough, and we can prevent things like ModelLoader(User, Company.id, User.name), which may cause bugs and hard to find because it won't throw errors.

UPDATE: in a second thought, we can throw error if the passed arguments don't exist in the model. Let me think over it.

[jekel]

@wwwjfy yeah, you are right, i want to be able to pass as column names strings and Column objects

and i also understood that there can be problem like you said

ModelLoader(User, Company.id, User.name)

but we can make sanity check that all columns passed belongs to the model?

do i understand correctly that AliasLoader would be used like AliasLoader(aliased_query, model)?

[wwwjfy]

Unfortunately no, only object returned by Model.alias(). I'll see what can be done to improve.

[jekel]

So... what do you suggest to use on aliased_query?

[wwwjfy]

For now, I'd say to use it like https://github.com/fantix/gino/pull/326/files#diff-f6247715790dd30f031c9c3d67d5814eR148, unless you have a very complex subquery, you may use TupleLoader or CallableLoader to process it or convert it to a non-model object.

[jekel]

@wwwjfy i have added SubqueryLoader and checks for Column belonging to used model

[wwwjfy]

For SubqueryLoader, this kind of usage, as mentioned, is already supported like this https://github.com/fantix/gino/pull/326/files#diff-f6247715790dd30f031c9c3d67d5814eR148 I prefer not using corresponding_column because Alias already has the column information, no need to involve the model class here.

As to pass Column to ModelLoader, I don't see major issue here. Small PRs are usually more welcomed because in this case, if it's separated, I may be able to merge that part.

[jekel]

For SubqueryLoader, this kind of usage, as mentioned, is already supported like this https://github.com/fantix/gino/pull/326/files#diff-f6247715790dd30f031c9c3d67d5814eR148 I prefer not using corresponding_column because Alias already has the column information, no need to involve the model class here.

I cant agree with you, it may work for simple queries where is only one model inside it, like you do in your tests, and only when using alias() on Model, not sqlalchemy Aliased query like in my usage case.

I will separate SubqueryLoader to another PR, so we can continue discussion there

[wwwjfy]

sorry, I wasn't clear. I didn't mean we shouldn't have SubqueryLoader, but in the use case in the new unit test, it's unnecessary. It'd be great if you can have a test case current AliasLoader can't get us the correct result.

[jekel]

@wwwjfy So, here is left only Columns objects support for ModelLoader

[wwwjfy]

Thanks!