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] SQL: Add all combinations of element refs and ids for 'create' methods #586

Open
EotT123 opened this issue May 9, 2024 · 8 comments

Comments

@EotT123
Copy link
Contributor

EotT123 commented May 9, 2024

Enhance the create methods for SQL objects by offering more flexibility for handling foreign keys. Currently, users have to choose between object references or IDs, but not a mix of them.

Example:

  • Currently:
create(Foo fooRef, Bar barRef, ...) // all by reference
create(Integer fooId, Integer barId, ...) // all by id
  • Wanted:
create(Foo fooRef, Bar barRef, ...)
create(Integer fooId, Integer barId, ...) // new: mix
create(Foo fooRef, Integer barId, ...)
create(Integer fooId, Bar barRef, ...) // new: mix
@EotT123
Copy link
Contributor Author

EotT123 commented May 9, 2024

PR which implements this: #576

@rsmckinney
Copy link
Member

Hi @EotT123. Thanks for the PR. My only reservation is the exponential growth of generated functions with FKs. Four or more FKs feels like too many. This is why I chose to have either all refs or all IDs. And use the all ID version when you don't have all the required FK refs. Calling getId() isn't so bad?

Perhaps I'll make this a configurable option... Support a boolean generatefullCreateApi dbconfig option, default to false to be on the conservative side.

What I really want is a union type:

create(Foo | Integer foo, Bar | Integer bar, ...)

But that's probably a bridge too far, for now.

@EotT123
Copy link
Contributor Author

EotT123 commented May 10, 2024

Thanks for your answer. I was also thinking about the exponential growth, and the fact that calling getId() isn't so bad. However, calling getId() is not always possible when the object is not yet persisted.
e.g.:

Foo foo = Foo.create(...);
Bar bar = Bar.create(foo, ...);
MyDB.commit();

In this example, calling Bar.create(foo.getId(), ...); would not work. Currently, the only way to make this work is to persist the foo object first, before persisting the bar object (but no rollback is done for foo when an error occurs during persisting bar).

The union type is a nice idea, but that's a complete new feature? And how would such a type be handled, because you don't know what the type is?

@rsmckinney
Copy link
Member

calling Bar.create(foo.getId(), ...); would not work

That can be made to work... In fact the information is there, it's just not made available until after the commit. Basically, after a create or update any generated data in the record is automatically retrieved and placed "on hold" in a private data section until the commit succeeds or fails. If it succeeds, the on-hold data becomes active, otherwise it is discarded.

I am considering delegating to on-hold/generated data during the commit. It's a straightforward change that probably should be done regardless of the outcome of this issue.

The union type is a nice idea, but that's a complete new feature? And how would such a type be handled, because you don't know what the type is?

Yep, it would be a new feature. The type can be identified with simple instanceof checks or the union type can provide a discriminant, which is a field to identify the type such as an enum. While I like union types, I don't really want to go down this path, at least not any time soon.

In any case I do like your PR, but I think it should be optional via dbconfig settings. I'll merge it and add the option and get a feel for it. I'll also probably experiment with making the on-hold data indirectly accessible during the commit.

@EotT123
Copy link
Contributor Author

EotT123 commented May 11, 2024

That might be a better solution. Then there even might not be the need to pass the objects by reference, only by id?

I'm a bit intrigued how you managed to do this. Autogenerated id's are generated by the database, when persisting the object. How do you already have access to the id's, before the object is persisted?

@rsmckinney
Copy link
Member

Well, there's no free lunch, but the price is low. Here's an example illustrating what I mean by making generated data available during the commit.

Foo foo = Foo.create(...);
MyDatabase.addSqlChange(ctx -> Bar.create(foo.getId(), ... ));
. . .
MyDatabase.commit();

During a commit addSqlChange calls force the tx to execute CRUD calls such as create and build that precede it. This behavior guarantees the sequence of CRUD changes are reflected in the tx as direct SQL ops execute. Thus, we can use addSqlChange as the basis for accessing generated data, which is already stored in entity bindings, hence foo.getId() can work by delegating to the "on hold" generated data internal to all manifold entity bindings.

The addSqlChange call is the price of the "free lunch". It works, but it's not intuitive. So another tweak to the changes I'm considering is to make CRUD operations execute eagerly instead of in batch when in a addSqlChange or commit call.

MyDatabase.commit(ctx -> {
    Foo foo = Foo.create(...);
    Bar.create(foo.getId(), ... );
    . . .
} );

With eager execution instead of batching INSERT ops we execute them with create calls, thus the INSERT corresponding with Foo.create() executes before Bar.create() enabling foo.getId().

Still not a free lunch as it must execute in a commit() call. Personally, I prefer having more fluid, tx-independent CRUD with the ability to commit changes that span code contexts, requests, etc. But this change is nice when a commit can be gathered in one place.

Probably more than you wanted to know, but that's the gist of it.

@rsmckinney
Copy link
Member

Maybe another way to understand this, your example involving refs works by virtue of the "on hold" bindings.

Foo foo = Foo.create(...);
Bar bar = Bar.create(foo, ...);
MyDB.commit();

Here the foo reference works because, although INSERTS are batched, the "on hold"/generated bindings are late bound and accessible during the INSERT for bar. This is primarily how the either/or API on create & build get by. Most it suffices, but there are plenty enough use-cases where only IDs are available. While fetch calls fill the gaps, they are less efficient than using straight IDs when you have'm.

Ok. Time for a weekend!

@EotT123
Copy link
Contributor Author

EotT123 commented May 11, 2024

Thanks for the explanation. I was thinking about something like it, but I was wondering if I maybe was missing something. All seems logical now. Thanks.

Probably the following would work too (although this is also not an optimal solution, and it doesn't solve anything, as we need separate methods for this to, same as what the PR adds.):

Foo foo = Foo.create(...);
Bar.create(foo::getId, ... );

In this example, getting the id is function, which is lazily executed, when the id is available.

Have a nice 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

No branches or pull requests

2 participants