#127 Adding basic procs that automatically fetches all "Many" entries of a "One-to-many" relationship #130
Conversation
The supplied functions allow fetching all related entries of a one-to-many relationship or a many-to-many relationship. It does so through the helper function "getRelatedFieldNameOn" That generic function can figure out, at compile time, whether model B has a field which is a foreign key to model A.
What is lacking is support for postgres as well as documentation. That can/will be delivered if the initial concept finds support
Effectively, moved many-to-one and many-to-many tests into one "related" file. That way it's easier to have its corresponding code in postgres in a separate file. Also the code itself was moved into the sqlite lib. I realized that some differences in Postgres syntax make it complex to support both it and sqlite in the same proc. So now the plan is to add postgres support by copy-pasting the sqlite procs and adjusting them accordingly.
|
I've come the realization that there's an important limitation I overlooked: It might be a good idea to check for that and throw a sensible exception at compile time. It already errors out at compile time, thankfully, the error message is just comparatively cryptic. Here an example:
|
|
@PhilippMDoerner Sorry for the slow response. I've been too busy to invest time in Norm lately. Finally, I've found the time to review your PR. I've left some comments, please see. Note that I'm not repeating comments for SQLite if it already has been said for Postgres. Also, if I suggest adding dostrings to one proc, it means all new procs should have dostrings. |
|
Another unfortunate edge case is that this does not deal properly with models that have 2 foreign key fields to the same model.
This means that Alternatively there could be an overload for The same is true for Edit: @moigagoo I am not seeing any comments |
src/norm/model.nim
Outdated
|
|
||
| raise newException( | ||
| FieldDefect, | ||
| "Tried getting foreign key field from model '" & name(sourceModel.type()) & "' to model '" & name(targetModel.type()) & "' but there is no such field!" |
There was a problem hiding this comment.
Since strutils is already imported, you can replace that with a more readable "$# $#" % [foo, bar] syntax.
There was a problem hiding this comment.
sourceModel.type is essentially just M, no need to make the type call.
There was a problem hiding this comment.
Since strutils is already imported, you can replace that with a more readable "$# $#" % [foo, bar] syntax.
Funnily enough, a couple days after I made this PR I've also gone over to using formatstrings. Do you have a heavy preference for the $# syntax or would using the strformat lib also work for you (should have no performance impact since this all works at compile time)?
As a sidenote, using strformat will be unavoidable for the postgres module. I've tried doing it with strutils and it promptly refused to pass the tests for reasons I don't think I fully get, but have to assume relate to the fact that postgres uses $1 etc for placeholders.
There was a problem hiding this comment.
@moigagoo
I fixed that and restructured the proc a bit. Previously this would have very poorly understandable behaviour if you had a model A with 2 FK fields to model B. It would secretly hand you the first and ignore the second one. Now there's a way to specify which field name you want to use for the selection. getRelatedFieldNameTo is now only used for an optional convenience proc (an overload of selectOneToMany) if you don't want to specify that field name and want norm to infer it for you.
I used strformat for this one though as I personally prefer it over strutils formatting syntax.
src/norm/model.nim
Outdated
| @@ -114,3 +114,25 @@ proc checkRo*(T: typedesc[Model]) = | |||
| when T.hasCustomPragma(ro): | |||
| {.error: "can't use mutating procs with read-only models".} | |||
|
|
|||
| proc getRelatedFieldNameOn*[T: Model, M: Model](targetModel: typedesc[T], sourceModel: typedesc[M]): string {.raises: [FieldDefect].} = | |||
There was a problem hiding this comment.
I'm not entirely happy with the proc name, it's hard to tell what it does.
It takes two types, T and M. We assume M is an object type with a field of type T. The function returns the name of the field in M that has the type T.
If this is correct, I have several suggestions and questions:
- It's safer to replace M with an instance of M because generally speaking you can't get fields of a type; you can get fields of a type instance. With the current approach, line 118 won't work if M has a field of type DateTime.
- What happens if there are several references to T in M? That means, M relates to T in several ways. How is that interpreted?
- The signature should probably be
func fieldsOfType[T: Model, M](obj: T, typ: typedesc[M]): seq[string]. The usage then would look like this:assert fidoTheDog.fieldsOfType(Person) == @["primaryOwner", "coowner"]. - The naming in 3 suggests this is a general-purpose proc that works with any type, not necessarily
Model. However, this may not be something you want. In this case, I suggestfunc fieldsOfModelType[T: Model, M: Model](obj: T, modelType: typedesc[M]): seq[string].
There was a problem hiding this comment.
It takes two types, T and M. We assume M is an object type with a field of type T. The function returns the name of the field in M that has the type T.
Almost, it's capabilities are in fact a bit broader than that. The call getRelatedFieldNameTo(S, T) returns the name of the field in S that "points to the same table that model T has". Which means this also works if S has a field with an fk-pragma that points to T. And it also works if S has a field of typeT2, if T2 has the same tablename as T via the tableName pragma. (With the most recent changes I also added the "If there is more than 1 field, then don't compile caveat).
- Using objects will not be possible here as the proc acts at compile time and I'm not aware how you would have objects at compile time for analysis. I'm personally insistent on keeping all of these calculations at compileTime, because the earlier you see these issues the better. If I can avoid a user running into an issue at runtime by having their code not compile when they do something wrong, I'd rather do that with a nice error message than let them notice at runtime.
Regarding Datetime: Fascinatingly, it doesn't pose a problem. I assumed the same but tried it out in case it worked and found that DateTime does not cause issues here! Likely because this all acts at compileTime. I'll attach a code example to the bottom of this comment so you can run it and see it yourself. Here a link to that code in play nim lang. - You get the first field that matches, exactly as you assumed. As such I changed things around a bit. The core proc
selectOneToManynow takes an additional parameterforeignKeyFieldNameto circumvent exactly the problem you lined out there. This basically makes this proc almost obsolete. However, I added an overload ofselectOneToManythat is without the parameterforeignKeyFieldNameparameter and instead usesgetRelatedFieldNameToto infer it. You can still run into this scenario of 2 foreign key fields to the same model, however you'll get a FieldDefect at compile time with a nice error message if you do. See the test"When there is multiple many-to-one relationships between two models and the type field for fetching the desired relationship is not specified, then do not compile"(postgres/trelated.nim line 88). - Sadly not possible, doing so will cause the code to not compile, as the proc does have side-effects. I think the compiler counts the FieldDefects that might be raised as such side-effects.
- I agree that the naming isn't great.
fieldsOfModelTypewould be inaccurate though, as you only get a single foreign-key-field on the source model pointing to the target model. Strictly speaking even my name is accurate, since the model-field onsourceand the model specified intargetdon't even have to match, they merely need to point to the same table. Of course I could change that, but that feels like narrowing the capabilities of this proc down unnecessarily. Anyway, I agree that the name is bad, I am however also not satisfied withfieldsOfModeltype. If I name it how I use it I guess it would beinferRelatedFieldNameToModelor something like it, but I'm not convinced of that name either.
import norm/[model, pragmas]
import std/[typetraits, strformat, options, times, macros]
proc getRelatedFieldNameTo*[M: Model](targetTableName: static string, normModel: typedesc[M]): string {.compileTime.} =
## A compile time proc that searches the given `normModel` type for any
## foreign key field that points to a table with the given name `targetTableName`.
## Raises a FieldDefect at compile time if the model does not have exactly
## one foreign key field to that table.
var fieldNames: seq[string] = @[]
const name = typetraits.name #Allows this generic proc to always have typetraits.name proc available even when the context it is called from doesn't import typetraits
for sourceFieldName, sourceFieldValue in M()[].fieldPairs:
#Handles case where field is an int64 with fk pragma
when sourceFieldValue.hasCustomPragma(fk):
when targetTableName == sourceFieldValue.getCustomPragmaVal(fk).table():
fieldNames.add(sourceFieldName)
#Handles case where field is a Model type
elif sourceFieldValue is Model:
when targetTableName == sourceFieldValue.type().table():
fieldNames.add(sourceFieldName)
#Handles case where field is a Option[Model] type
elif sourceFieldValue is Option:
when sourceFieldValue.get() is Model:
when targetTableName == genericParams(sourceFieldValue.type()).get(0).table():
fieldNames.add(sourceFieldName)
if fieldNames.len() == 1:
return fieldNames[0]
elif fieldnames.len() < 1:
let errorMsg = fmt "Tried getting foreign key field from model '{name(M)}' to model '{targetTableName}' but there is no such field!"
raise newException(FieldDefect, errorMsg)
elif fieldnames.len() > 1:
let errorMsg = fmt "Can't infer foreign key field from model '{name(M)}' to model '{targetTableName}'! There is more than one foreign key field to that table!"
raise newException(FieldDefect, errorMsg)
proc getRelatedFieldNameTo*[S: Model, T:Model](source: typedesc[S], target: typedesc[T]): string {.compileTime.} =
## A compile time proc that analyzes the given `source` model for any foreign key field that points to
## the table of the `target` model. Raises a FieldDefect at compile time if the model does not
## have exactly one foreign key field to that table
result = getRelatedFieldNameTo(T.table(), source)
type A* = ref object of Model
time*: DateTime
type B* = ref object of Model
somename*: string
time*: DateTime
fkToA*: A
type C* = ref object of Model
da*: string
time*: DateTime
fkToA* {.fk: A.}: int64
type D* = ref object of Model
du*: string
time*: DateTime
fkToA*: Option[A]
const val1 = B.getRelatedFieldNameTo(A)
const val2 = C.getRelatedFieldNameTo(A)
const val3 = D.getRelatedFieldNameTo(A)
echo val1
echo val2
echo val3
src/norm/postgres.nim
Outdated
|
|
||
| # One-to-Many Fetching | ||
| proc selectOneToMany*[O: Model, M: Model](dbConn; oneEntry: O, relatedEntries: var seq[M]) = | ||
| const foreignKeyFieldName: string = O.getRelatedFieldNameOn(M) |
There was a problem hiding this comment.
- Please combine multiple const, let, and var definitions in one block.
- Type declarations are unnecessary.
- If you cover the case of one model referencing another one in several fields, this will have to change to handle the case of many fields.
There was a problem hiding this comment.
- Does this still apply after the revision?
- I do this mostly in my own codebases since intellisense in vscode takes forever and I want to be able to see the types at glance if it's not immediately painfully obvious what they are. I removed them from "manyTableNames" and when defining "sqlCondition" since one could argue it should be obvious what type they return.
- I covered this case by changing this proc to also receive a
foreignKeyFieldNameparameter and an overload that infer that parameter for me in a nieche scenario, for convenience. One could argue if that nieche scenario is even desired or not. If not, I could throw out that overload as well asgetRelatedFieldNameTo.
src/norm/postgres.nim
Outdated
|
|
||
| # One-to-Many Fetching | ||
| proc selectOneToMany*[O: Model, M: Model](dbConn; oneEntry: O, relatedEntries: var seq[M]) = | ||
| const foreignKeyFieldName: string = O.getRelatedFieldNameOn(M) |
There was a problem hiding this comment.
Note the here you extract the type to pass to getRelatedFieldNameOn only to instantiate it again in the proc. You could just pass the first element of relatedEntries to the proc.
There was a problem hiding this comment.
I don't think it would work at compile time in such a case, which is when getRelatedFieldNameOn acts (now renamed to getRelatedFieldNameTo and likely renamed again once we can agree on a name) .
I always forget to click Sumbit review 😞 Sorry. |
It now has specified behaviour for when a Model has 2 related fields to a given table. Also added a convenience proc that has the same signature as before and did some renaming
|
@moigagoo Finally got around to writing some nim again. I went through the comments and adjusted what I could and fixed yours as well as my own suggestions. That is with the exception of my second comment. Still haven't found something I'm 100% satisfied with to solve that one, though it's a minor gripe since at least it doesn't allow you to compile wrong code. |
The proc had no chance of dealing with joinModels with multiple FK fields to the same table. This was fixed by allowing the desired fields for use to be specified.
|
With the merges of develop into this branch I would consider this done so far. @moigagoo I'd request feedback or rejecting the PR if the code provided here is not moving in the direction you would want. |
|
@PhilippMDoerner I'll be honest with you. I probably won't have time to review the PR any time soon. I'm from Russia, and things aren't going well. My entire life is about to change dramatically. Really sorry to be the showstopper but I just can't invest any time in pet projects now. |
No worries, I can't claim to understand, I'm merely a horrified bystander for the most part of it, but I can still wish you best of luck. May peace follow hopefully soon. |
|
@PhilippMDoerner could you please update the changelog with your changes? I'll merge the PR after that. |
|
@moigagoo Holy damn! Really nice to hear from you! Given the situation and your last comment I was starting to fear the worst! Thank you very much for the response and especially for noticing the similarity in macro. Given the documentation on "dot", I think their intent is identical, but I believe they have subtle differences that become apparent during usage. I say that mostly because I tried replacing one with the other, but the tests didn't run through in that scenario. I'll take a look whether I can find a way to move this all to "dot" and spare ourselves the second macro (aka I'll ask on the discord because for the most part I still don't get macros. There's a reason I use compile time pragma on procs). If I can't make that possible, I'll likely move the "getField"-macro and "hasField" template into the "dot" file as they both perform very similar functions, seems better structurally. Either way, I'll get to writing those changelog edits. |
|
@moigagoo I think the fact that dot is more specifically geared towards ref objects somehow makes it impossible to use "dot" instead of "getField". |
|
Added changelog updates, should be all there then. Once this is merged I'll start another issue (and subsequently a PR) for writing docs |
This code adds basic procs to sqlite to fetch the other side of a "one-to-many" or a "many-to-many" relationship. It does so through generic procs that write the basic sql condition of the query for you on the related table you're fetching stuff on. In the case of many-to-many, it even unpacks the seq[joinTableModel] for you into seq[YourDesiredModel]. This unpacking is done via macro.
All the while, it manages to block you at compile time from using these procs incorrectly. If you try to use them with 2 models that do not have a many-to-one relationship, or 3 models that do not repesent a many-to-many relationship, your code will not compile.
getRelatedFieldNameOnfigures that one out at compile time for you, as it acts almost entirely on the types.The other 2 procs,
selectOneToManyandselectManyToManyalso figure out the sqlCondition to write at compile time, though the way they do it differs slightly between postgres and sqlite.Particularly regarding naming of variables and more I'm very open to feedback, as at some point the ideas for naming things better just disappeared, which imo is very visible in my proc names but also variable names used within them. So I'm very much aware that this likely still needs some naming works, or where which could should be moved, but I believe this does encompass a basis to start from.
As for the direct feedback from the feature discussion:
Done (That was more of a leftover from when I wrote this code in my own side-project and not quite intended for the norm library, I apologize for not cleaning that up)
That macro has been renamed
unpackFromJoinModel. I tried a version of it as template before, but got stuck with that as I wanted to pass the field name as a string since that allowed me to use it with the output fromgetRelatedFieldNameOn. I'm not quite sure how else to do it, though would agree that a proc or template would be preferable, simply because they're easier to maintain.Done
For
getRelatedFieldNameOnI sadly found no other way. Luckily, forgetRelatedFieldNameOnthat is actually perfectly fine, as it does not deal with field values and acts at compile time. I explicitly addedDateTimeto theDoctormodel which I use in one of the tests, which passes. I did however update the two other procs (selectOneToManyandselectManyToMany) to work with model instances instead of types though.I couldn't make a version of
getRelatedFieldNameOnact at compile time while usingisModel. I may have done something wrong, I can only say that I didn't manage to get that approach working while doing the things I wanted to do at compile time.Same as 1, that was on me and I fixed it accordingly. Sorry for the confusion I caused there.