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

Inheritance #128

Closed
jonathanstowe opened this issue Mar 15, 2019 · 8 comments
Closed

Inheritance #128

jonathanstowe opened this issue Mar 15, 2019 · 8 comments

Comments

@jonathanstowe
Copy link
Contributor

jonathanstowe commented Mar 15, 2019

Attempting to create a row from a model that inherits all its attributes from another model doesn't work:

use Red;


model Bar { ... }

model Foo {
    has Int $.id    is serial;
    has Str $.name  is column;
    has Bar @.bars   is relationship({ .foo-id });
}

model Bar {
    has Int $.id    is serial;
    has Str $.name  is column;
    has Int $.foo-id is referencing({ Foo.id } );
    has Foo $.foo is relationship( { .foo-id });
}

model ChildFoo is Foo { }
model ChildBar is Bar { }

my $*RED-DEBUG          = $_ with %*ENV<RED_DEBUG>;
my $*RED-DEBUG-RESPONSE = $_ with %*ENV<RED_DEBUG_RESPONSE>;
my $*RED-DB             = database "SQLite", |(:database($_) with %*ENV<RED_DATABASE>);


Foo.^create-table;
Bar.^create-table;

my $foo = ChildFoo.^create(name => "test");

Results in

P6opaque: no such attribute '$!id' on type ChildFoo in a ChildFoo when trying to bind a value
  in submethod TWEAK_pr at /home/jonathan/devel/perl6/3rdparty-modules/Red/lib/MetamodelX/Red/Dirtable.pm6 (MetamodelX::Red::Dirtable) line 61
  in method create at /home/jonathan/devel/perl6/3rdparty-modules/Red/lib/MetamodelX/Red/Model.pm6 (MetamodelX::Red::Model) line 239
  in block <unit> at inherit line 30

Which is somewhat less than awesome. On reflection it is clear why it doesn't work and that it will be far from simple to make it do what one would expect, but it might be nice while it doesn't work to bail out at compile time (perhaps by providing a multi trait_mod:<is>(Mu:U $model, Model:U $parent) or something more specific) so people don't spend a while trying to work out what it is that they have done wrong.

Of course it would be nice if it could be made to work and it might be simpler than I think it is.

Right now I'm only thinking about class inheritance rather than table level inheritance that is available in say, Pg or Oracle, but that would logically flow from the former if it can be made to work.

@FCO
Copy link
Owner

FCO commented Mar 15, 2019

Maybe it could work if we change: nqp::bindattr(nqp::decont(instance), self.WHAT, col.name, proxy); to something like col.set_value: instance<>, proxy but I can't test it now... :(

FCO pushed a commit that referenced this issue Mar 15, 2019
@FCO
Copy link
Owner

FCO commented Mar 15, 2019

use Red;


model Bar { ... }

model Foo {
    has Int $.id    is serial;
    has Str $.name  is column;
    has Bar @.bars   is relationship({ .foo-id });
}

model Bar {
    has Int $.id    is serial;
    has Str $.name  is column;
    has Int $.foo-id is referencing({ Foo.id } );
    has Foo $.foo is relationship( { .foo-id });
}

model ChildFoo is Foo { }
model ChildBar is Bar { }

my $*RED-DEBUG          = $_ with %*ENV<RED_DEBUG>;
my $*RED-DEBUG-RESPONSE = $_ with %*ENV<RED_DEBUG_RESPONSE>;
my $*RED-DB             = database "SQLite", |(:database($_) with %*ENV<RED_DATABASE>);


Foo.^create-table;
Bar.^create-table;
ChildFoo.^create-table;
my $foo = ChildFoo.^create(name => "test");

is working... everything else isn't... :(

@jonathanstowe
Copy link
Contributor Author

Yeah I was going to say doesn't it need the bind for the closure on the proxy to work right?

@jonathanstowe
Copy link
Contributor Author

Almost 👍

Slightly amended:

use Red;


model Bar { ... }

model Foo {
    has Int $.id    is serial;
    has Str $.name  is column;
    has Bar @.bars   is relationship({ .foo-id });
}

model Bar {
    has Int $.id    is serial;
    has Str $.name  is column;
    has Int $.foo-id is referencing({ Foo.id } );
    has Foo $.foo is relationship( { .foo-id });
}

model ChildBar { ... }
model ChildFoo is Foo is table('foo') { 
    has ChildBar @.bars   is relationship({ .foo-id });
}
model ChildBar is Bar is table('bar') { 
    has ChildFoo $.foo is relationship( { .foo-id });
}

my $*RED-DEBUG          = $_ with %*ENV<RED_DEBUG>;
my $*RED-DEBUG-RESPONSE = $_ with %*ENV<RED_DEBUG_RESPONSE>;
my $*RED-DB             = database "SQLite", |(:database($_) with %*ENV<RED_DATABASE>);


Foo.^create-table;
Bar.^create-table;

my $foo = ChildFoo.^create(name => "test");

my $bar = $foo.bars.create(name => "test-bar");
say $bar.perl;

Gives rise to:

QL : CREATE TABLE foo(
   name varchar(255) NOT NULL ,
   id integer NOT NULL primary key AUTOINCREMENT
)
BIND: []
SQL : CREATE TABLE bar(
   foo_id integer NULL references foo(id),
   id integer NOT NULL primary key AUTOINCREMENT,
   name varchar(255) NOT NULL 
)
BIND: []
SQL : INSERT INTO foo(
   name
)
VALUES(
   ?
)
BIND: ["test"]
SQL : SELECT
   foo.name , foo.id 
FROM
   foo
WHERE
   _rowid_ = last_insert_rowid()
LIMIT 1
BIND: []
Ambiguous call to 'bars(ChildFoo: )'; these signatures all match:
:(Mu:D: *%_)
:(Mu:D: *%_)
  in block <unit> at inherit line 37

Which gives rise to some thoughts. I've got yo pop out for a bit but I'll flesh that out later.

@jonathanstowe
Copy link
Contributor Author

So really, for me, this raises a few uncertainties as to what the correct behaviour should be.

Firstly I'm not sure whether it should by default use the same table name or whether it should be a new table name and if the Driver supports it do something like:

CREATE TABLE foo(
   id integer NOT NULL primary key AUTOINCREMENT,
   name varchar(255) NOT NULL 
);

CREATE TABLE child_foo(
...
) INHERITS (foo);

Though emulating the behaviour of queries on foo for a DBMS that doesn't support inheritance might be a challenge if the behaviour is to be consistent (which IMO it should be.) (see e.g. https://www.postgresql.org/docs/10/tutorial-inheritance.html regarding the inheritance in Pg.)

In either case I think the ability to inherit without table-level inheritance while specifying a table name should be retained as that provides a simple way of over-riding a table name that might be defined in some 3rd party module.

Secondly, I'm not convinced that over-riding the relations to return the right type of object is the best thing to do here as the relation may be defined in some other module for instance, I'm wondering whether another more general mechanism to do some kind of type mapping on a per model basis might work.

@FCO
Copy link
Owner

FCO commented Mar 16, 2019

I think for now I’ll make it break with a beautiful error until we think how it should behave...

Sent with GitHawk

@jonathanstowe
Copy link
Contributor Author

Before you do that could you make a branch of the somewhat working code so I can continue to tinker with it?

@vrurg
Copy link
Contributor

vrurg commented Apr 25, 2019

It is now my turn to try the inheritance and fail. I would like to propose the following behavior which, I think, would be as close to DWIM as possible.

A model inherits table name of its parent if that was set explicitly with is table trait. Otherwise the name default to the child node name:

model BaseModel is <a_table> { ... }
model MyMode is BaseModel { ... } # maps to «a_table»

model Foo { ... }
model Bar is Foo { ... } # maps to «bar»

I think this is a way to specify wether we act on a concrete or an abstract model. Of course, child can always specify own table name.

It would also make it possible to use only a subset of columns from a big table for one task while allow to start using additional columns for another task without the need to re-define the already existing functionality.

model MyModel is BaseModel {
    has Str $.additional-col is column;
}

And especially this would be the best way to handle a situation when the parent model doesn't need to know about a third-party table which could be referencing the parent. In this case a child model would be used just to introduce the references from the third-party:

model MyModel is BaseModel {
    has ThirdParty @.records is relationship{ .base-id };
}

FCO pushed a commit that referenced this issue Nov 7, 2020
@FCO FCO closed this as completed Jan 9, 2023
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

3 participants