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

[Feature Request] Allow specifying Read-Only Models #125

Closed
PhilippMDoerner opened this issue Jan 3, 2022 · 7 comments · Fixed by #126
Closed

[Feature Request] Allow specifying Read-Only Models #125

PhilippMDoerner opened this issue Jan 3, 2022 · 7 comments · Fixed by #126
Labels
enhancement New feature or request

Comments

@PhilippMDoerner
Copy link
Collaborator

PhilippMDoerner commented Jan 3, 2022

Heyho,

I use norm as an interface to an already existing database that was created by Django (I'm effectively reimplementing an already existing backend).
The way I use norm includes having multiple models on the same table, some with their FK fields resolved into the full data of those other entries, some where various fields are left out because they aren't needed.
This allows inserts with only the minimal amount of data necessary (So no entire models of linked fields necessary), while also giving me the ability to have "slices" of data from my database instead of the whole thing, avoiding unnecessary data fetches etc.

Right now, if I used one of these models and tried to instantiate an entry with it, I would run into a DbError at runtime.
It would be beneficial to move this problem to compile time by providing an alias to the Model type, that can do all the same things as Model but does not have methods whose signature allow for insert/update/delete operations with them.

Without the necessary procs, a developer using the library would be notified at compile time that he's doing something wrong by creating/updating/deleting with a read-only model.

I'm not sure how much effort or complexity this ads, because I'm not sure how such a model would e.g. need to handle ids.
Does this seem sensible to you as well?
If yes, are there any things are possible problems I might be overlooking?

As an example:
Let's say I have a model of a character that represents the actual table "character":

type Character* {.tableName: CHARACTER_TABLE.} = ref object of Model
    ##[TableModel of the table of story characters. ]##
    player_character*: bool #Whether the character is controlled by a player or not
    alive*: bool
    name*: string
    gender*: string
    race*: string
    title*: Option[string] # A title the character might have, e.g. "Lord", "Duke", "Doctor"
    description*: Option[string] # A description of the character
    current_location_id*: Option[int] # The id of the location at which the character is currently located
    creation_datetime*: DateTime
    update_datetime*: DateTime
    campaign_id*: int # The id of the campaign that this character occurred in

and let's say there's scenarios where I want to display just a list of characters. To save myself performance and unnecessary data fetches, I can just define an "overview" model with only the fields I would want for such a list:

type CharacterOverview* {.tableName: CHARACTER_TABLE.} = ref object of Model
    ##[A reduced Model of a story-character. Cut down to the bare minimum needed to
    make a list of characters with necessary meta data ]##
    player_character*: bool
    alive*: bool
    name*: string
    update_datetime*: DateTime

Right now I could try to make a row in the table "character" with that CharacterOverview model. I am avoiding this right now through naming schema and documentation (well and by knowing how I intend to use them), but ideally I should never have even the possibility to make this mistake.

@moigagoo
Copy link
Owner

moigagoo commented Jan 4, 2022

I've been thinking exactly about the same thing just yesterday :-)

There are several ways we can do that:

  1. Introduce ro pragma for types that, when present, would prevent execution of any proc except for select. The syntax is nice but the semantics is bad since we're still catching the error at runtime, it's just a less harsh error.
  2. Introduce a new type View that would only allow Model and View type fields. Then, introduce createView and select(View). We'd get true view support from that and I'm not sure this is what we want in this feature request 😁
  3. Introduce new type ReadOnlyModel and inherit Model from it, then redefine select to apply only to ReadOnlyModel.

@moigagoo
Copy link
Owner

moigagoo commented Jan 4, 2022

I'm leaning towards 2, as it seems logical to use views to represent read-only data.

@PhilippMDoerner
Copy link
Collaborator Author

It's a pretty tough choice, I'll give you that.

I'm prone to three, though none of the choices are flawless.

One is the way Django does it, but that acts at runtime and comes with a (minor) performance penalty if you're doing if-checks for all models that come in.

As for three, that one runs into naming issues. Not that this is a large problem mind you. If you were to inherit Model from ReadOnlyModel, the way the names are chosen results in slightly unexpected behaviour. That is, since "ReadOnlyModel" has the extra words "ReadOnly" bolted on top of "Model", you would expect that it inherits from Model, not the other way around. The semantics are a bit confusing. I've been playing around with the idea of maybe having a third object BaseModel/DatabaseEntity or sth that both ReadOnlyModel and Model inherit from, but that comes with its own set of possible problems and complications down the line, were you to add view support (since you'd expect that that would also inherit from that third object type).

That's why I thought a type alias might be a solution to avoid this, but that seems to come with its own problems (I assume).

The issue I see with 2 is that strictly speaking the concepts of a view and sth being read-only in SQL are not automatically linked. So normally, you can create/update/delete through a view in the underlying tables. Since views can be used that way, if you inherently combine them with "read-only", you're automatically also limiting the SQL feature set you can make available through the ORM. If you then wanted to add create/update/delete ability to views at some point in the future, you'd likely be changing your API around views.

... man, I kinda wish nim had interfaces right now.

@moigagoo
Copy link
Owner

moigagoo commented Jan 4, 2022

As for three, that one runs into naming issues.

I totally get the naming issue, it bothers me too. And workarounds like having BaseModel and EditableModel or something looks really forced.

The issue I see with 2 is that strictly speaking the concepts of a view and sth being read-only in SQL are not automatically linked.

This is true, but using ORM inevitably means imposing some limitation over raw SQL. That's the price you pay for convenience.

Maybe, naming it View would be a misnomer. How about Slice?

If you then wanted to add create/update/delete ability to views at some point in the future, you'd likely be changing your API around views.

Well, we'll just have to implement delete/update/insert for View type ¯\(ツ)/¯ Doesn't seem to break anything.

@PhilippMDoerner
Copy link
Collaborator Author

PhilippMDoerner commented Jan 4, 2022

I totally get the naming issue, it bothers me too. And workarounds like having BaseModel and EditableModel or something looks really forced.

Complete agreement on my part.

Well, we'll just have to implement delete/update/insert for View type ¯(ツ)/¯ Doesn't seem to break anything.

But then View wouldn't be Read-Only anymore.
It would need a new type, one of which then could be a Read-Only-View and the other a View that allows CRUD operations.

And then either you model with the name "View" stays "Read-only" at that point and the new type would likely have a name like "CRUDView" and your then your behaviour in Views is in complete contrast to "ReadOnlyModel" to "Model".

Or "View" becomes able to create/update/delete, then the new type would likely have a name like "ReadOnlyView" at which point your naming is consistent again, but your API changes and those that used View before would need to move to "ReadOnlyView" to limit their model in the same way as before.

I'm personally leaning towards 3 at this point I think and just accepting the minor naming issue, which I then should likely write about in docs. But this is just my opinion and definitely not my final choice to make.

@moigagoo
Copy link
Owner

moigagoo commented Jan 4, 2022

I think I've found a great solution. We can use ro pragma and check it during compilation.

Check this out:

import macros


template p {.pragma.}


type
  Base = ref object of RootObj
  Foo {.p.} = ref object of Base
  Doo = ref object of Base


proc s[T: Base](b: T) =
  when b.hasCustomPragma(p):
    {.error: "Fail".}

  echo "Success"


let
  f = Foo()
  d = Doo()

s(f)
s(d)

This won't compile because Foo has p pragma. This is exactly what we want: insert called with an instance of Model {.ro.} would not compile.

And this solution is cheap: just add a two-line template that does the check and use it in insert, delete, and update.

Sounds good?

P.S. How would interfaces help in this case?

@moigagoo moigagoo linked a pull request Jan 4, 2022 that will close this issue
@moigagoo moigagoo added the enhancement New feature or request label Jan 4, 2022
@moigagoo
Copy link
Owner

moigagoo commented Jan 4, 2022

@PhilippMDoerner pls see the linked PR for the implementation. I haven't added the tests yet but the code is already there.

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

Successfully merging a pull request may close this issue.

2 participants