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

Filter based on related tables' columns #250

Open
ligustah opened this issue Dec 25, 2017 · 7 comments
Open

Filter based on related tables' columns #250

ligustah opened this issue Dec 25, 2017 · 7 comments

Comments

@ligustah
Copy link
Contributor

Hello there,

I very much enjoy using kallax, so thanks for that!

I'm currently implementing a schema that looks like this:

type A struct {
	kallax.Model
	ID    int64 `pk:"autoincr"`
	C     *C `fk:",inverse"`
}

type B struct {
	kallax.Model
	ID    int64 `pk:"autoincr"`
	C     *C `fk:",inverse"`
}

type C struct {
	kallax.Model
	ID       int64 `pk:"autoincr"`
	Property int64
}

So there is a 1:1 relationship between A and C, and between B and C respectively.

Now maybe I'm just holding it wrong, but I cannot seem to find a way to query all A's that have a specific value for the Property of their related C. What I was trying to do is this:

aStore := NewAStore(db).Debug()
a, err := aStore.FindOne(
	NewAQuery().
		WithC().
		Where(kallax.Eq(Schema.C.Property, 5)),
)

When I run that, however, I get this behavior:

2017/12/25 11:18:14 kallax: Query: SELECT __a.id, __a.c_id, __c_C.id, __c_C.property FROM a __a LEFT JOIN c __c_C ON (__c_C.id = __a.c_id) WHERE __a.property = $1 LIMIT 1, args: [5]
2017/12/25 11:18:14 pq: column __a.property does not exist

So from what I can tell kallax is using the schema of A to build the qualified column name, which obviously doesn't work. Is there a way in kallax right now, that would allow me to run this query successfully?

Otherwise I was thinking about implementing a wrapper for the SchemaField that would always use a specific base schema, such that Schema.C.Property would always use the schema of C (which I assumed was the default behavior) or modify the code generation to always maintain this mapping.

Thanks in advance, and enjoy your holidays!

@roobre
Copy link
Contributor

roobre commented Dec 26, 2017

AFAIK this is not possible at the moment. You would need to select C's based on C.Property WithA() instead of the other way around.

@ligustah
Copy link
Contributor Author

From what I can tell C doesn't have the WIthA query option. To be more specific, what I actually need is to filter on both A and C. Is there a particular reason why the qualified name is built at query time and not pre-generated like most of the other things? And if not, would you accept a patch that implemented this?

@roobre
Copy link
Contributor

roobre commented Dec 27, 2017

As far as I know, the only purpose autogenerated code serves is as a more convenient API (this is, user-facing code). Core operations are performed on "static" (non-generated) code, using type-agnostic interfaces.

Kallax does not support what you are trying to acomplish, probably because a design choice: Even if querying according to parameters of two or more tables is a normal operation, from a relationship point of view is pretty rare. Most cases probably can be circunvented with an alternative design approach.

As for the patch you mention, I don't see at the moment how this feature could be implemented without: a), breaking existing APIs, b) breaking existing database schemas, c) relying on generated code. Please tell me if you devise a design which satisfies these requirements (with some diagrams maybe), and we can take a look and see if benefits outweigth the risks.

@ligustah
Copy link
Contributor Author

I see. Please just for me to fully understand the constraints for a possible implementation, is there any reason that this:

Where(kallax.In(Schema.C.Property, 5))

should not be resolved against the base schema of C in any case (such as when used in a query against schema A)? At least to me the documentation suggests that there are no special requirements regarding the context in which those fields are used.

Thanks a lot for your time, and for taking care of kallax in general!

@roobre
Copy link
Contributor

roobre commented Dec 30, 2017

So, if I understood correctly, your plan is to modify query builder to check whose property is the schema you pass to it, so it can generate the correct identifier. AFAIK, now the query constructor is blind to this, and it just assumes all schema fields refer to the model whose store is being used. Modifying this behaviour would require some refactoring here and there, and can get pretty huge.

Right now I'm quite busy so I won't be able to help. I think this is interesting, but the first thing on my TODO list is finish #114 implementation, and refactoring the query builder could negatively impact this goal.

I'm interested on this, as I think is a nice feature for kallax and seems both fitting and intuitive enough to me, but I'm afraid the need to perform this level of refactoring on the code would difficult the addition of more prioritary tasks. Also, this would require extensive testing, which would only delay more these urgent features.

I'm leaving this open for the near future, when we reach a less complicated state and refactoring here and there won't drive me down to the rebase hell.

@ligustah
Copy link
Contributor Author

Sure, sounds like a good plan! Holidays are over soon, so I don't really know how much time I'll have left, but I might try to implement a PoC for this (entirely to satisfy my curiosity :D)

@roobre
Copy link
Contributor

roobre commented Jan 4, 2018

Of course! I'll be happy to take a look into it :D

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

2 participants