Skip to content

Conversation

PhilippMDoerner
Copy link
Collaborator

@PhilippMDoerner PhilippMDoerner commented Aug 12, 2022

This proc adds the feature requested by #133 :
A way to interact with the database with arbitrary SQL, yet still benefit from norms incredibly useful parsing of Row to objects, just this time it allows for arbitrary object types instead of using models.

This has the following benefits:

  • It becomes possible to interact with views which do not have an id column, as the user can define their own types for interacting with the view
  • It becomes possible to use norm for executing arbitrary complex SQL such as recursive queries, which was not possible before

The specific procs that allows this are:

  • A modification of fromRowPos and fromRow which now can deal with any ref object as opposed to only Model types
  • rawSelect procs that make use of that and allow the user to pass non-Model ref object and a full SQL query to execute

A small side-benefit:
I observed around a 10% speed-up for my limited test-cases when executing the "new" version of fromRowPos. The reason for this speed-up is that some computation has been moved to compile-time instead of runtime.

Things that would still need to be done:

  • General review + Approval that this feature is wanted
  • Figure out in which module procs such as isContainer, toOptional and isEmptyColumn should be stored, as rowutils does not feel like the appropriate place for these (I've decided to put them into a shared utils.nim module in private)
  • Figure out actually decent names for isContainer and toOptional. These names are (in my mind) only placeholders since I couldn't think of a better name for now. They are more "open" alternatives to isModel and model. With the new context of using ref object instead of Model however isRefObject and refObject as names seemed confusing to me. (I've decided to go with toOptional and isRefObject)
  • Write tests for the rawSelect procs (and maybe the modified fromRowPos proc) ( @moigagoo I would proceed with this point and the ones below once we ironed out the other ones) (Basic tests are written. The details of the parsing into ref object do not need to be tested - the parsing from object to model already does that)
  • Add changelog changes (DONE)
  • Add nimibook docs

This now allows people to interact with the database
without having to bypass norm for more complex SQL.
@PhilippMDoerner
Copy link
Collaborator Author

@moigagoo before I do the (not that inconsiderable amount of) work of getting this PR into a "finished" state, I would like your confirmation first that the feature is actually wanted.
I'd like to avoid a scenario where I bring this to the finish line, only for us to reject the PR.

Due to the tables in postgres being created in a case-sensitive manner
they require quotation marks around them and must be case-sensitive correct.
This is only the case with postgres, not sqlite.
https://stackoverflow.com/questions/43111996/why-postgresql-does-not-like-uppercase-table-names
@PhilippMDoerner
Copy link
Collaborator Author

Since the actual conversion of Row -> ref object is implicitly covered by all the tests that convered from Row -> Model I personally think that no further tests are required. Just that they generally can work, which the trawselect.nim test files cover.

To me the work on this PR is thus done and it could be merged.

@PhilippMDoerner
Copy link
Collaborator Author

@moigagoo
Yo, it's been near a month since creation of the PR with request for comment and almost a week since it finished (which I've done to have a modified branch of norm that I can now use in my sideproject).

Is this to be on ice until the escape column name PR is merged or is something else going on?

Copy link
Owner

@moigagoo moigagoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's just one comment I have. Otherwise, I'm ready to merge.

@moigagoo
Copy link
Owner

moigagoo commented Sep 9, 2022

@PhilippMDoerner sorry for the slow responses. I've looked through the OR and found just one place where I have concerns. Please review this comment and we'll merge the PR.

@PhilippMDoerner
Copy link
Collaborator Author

PhilippMDoerner commented Sep 9, 2022

@moigagoo
I've found a new reason for concern that is related to your comment but in a different manner.
It turns out there are 2 pragmas that we would be unable to support with this:
Using T() makes it essentially impossible to use the requiresInit pragma with a model/ref object. noInit seems similarly affected though I did not test this one.
To me that is an acceptable sacrifice, but I thought I should bring it up since it came up in convo with Yardanico.

@moigagoo moigagoo merged commit 1b9078b into moigagoo:develop Sep 10, 2022
@moigagoo
Copy link
Owner

I'll release 2.5.2 Monday. I'm afk for the entire weekend.

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

Successfully merging this pull request may close these issues.

2 participants