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

Add support for load_namespaces(lazy_load => 1, ...) #32

Closed
wants to merge 3 commits into from
Closed

Add support for load_namespaces(lazy_load => 1, ...) #32

wants to merge 3 commits into from

Conversation

jhthorsen
Copy link

All tests successful.
Files=94, Tests=20844, 25 wallclock secs
Result: PASS

All database related tests was skipped when running this test suite.


batman: I'm sending this pull request so we don't forget about it ribasushi. comments are more than welcome

  All tests successful.
  Files=94, Tests=20844, 25 wallclock secs
  Result: PASS

  All database related tests was skipped when running this test suite.
@agranig
Copy link

agranig commented Sep 4, 2013

When using this patch (and only adding the lazy_load=>1 in my schema) within a Catalyst app, I get the warning

No sources found (did you forget to define your tables?)

on startup, and when using the schema, I get the error

$c->model('DB::myresult') did not return a resultset. Did you set user_model correctly?

Am I expected to do something specific in my model to make this work?

@jhthorsen
Copy link
Author

Not really sure how to fix that, since it's a warning from https://metacpan.org/source/ILMARI/Catalyst-Model-DBIC-Schema-0.61/lib/Catalyst/Model/DBIC/Schema.pm#L536

As a temp hack, you could probably monkey patch the _install_rs_models() method.

But getting $c->model('DB::myresult') to work will probably be harder, since it relies on the source info to be loaded. Just do $c->model('DB')->resultset('myresult') instead or make a new helper method (or whatever it's called in Catalyst)

@agranig
Copy link

agranig commented Sep 4, 2013

Hi, I actually do $c->model('DB')->resultset('myresult') throughout my code, except for the authentication config, where I have:

            ...
            store => {
                class => 'DBIx::Class',
                user_model => 'DB::myresult'
            ...

I'll check if I can work around this somehow.

@dbsrgits-sync
Copy link

On Wed, Sep 04, 2013 at 08:25:45AM -0700, Andreas Granig wrote:

When using this patch (and only adding the lazy_load=>1 in my schema) within a Catalyst app, I get the warning

No sources found (did you forget to define your tables?)

on startup, and when using the schema, I get the error

$c->model('DB::myresult') did not return a resultset. Did you set user_model correctly?

Am I expected to do something specific in my model to make this work?

No, this is very much alpha code. The final version will not, I repeat
will not require any changes to your Catalyst app. An update should
follow up some time next week, I want to finish
#29 before moving on to this.
Sorry for the delays :(

@dbsrgits-sync
Copy link

On Wed, Sep 04, 2013 at 09:19:34AM -0700, Jan Henning Thorsen wrote:

But getting $c->model('DB::myresult') to work will probably be harder

Then your initial design needs to be rethought ;) Note - "the minimum
amount of changes that will somewhat work" will not fly in this case.
The lazyfier needs to be 99.9% compatible with existing code bases to be
worthwhile for inclusion. Anyway off to fix my own crap ;)

Cheers

@jhthorsen
Copy link
Author

@dbsrgits-sync: I don't understand how you can make some automatic model creation in Catalyst::Model::DBIC::Schema DBIx::Class's problem. I haven't read the source code for C::M::DBIC::Schema in a while, but from what i remember it automatically generates MyApp::Model::DB::TableA, ::TableB, ... based on the sources available on load time. If that is not changed...how will you be able to "eager create" namespaces based on "not loaded sources" ??

@ribasushi
Copy link
Collaborator

Well... allow me to explain I guess.

I am making this DBIC's problem because this is a simple "test my dependents" kind of problem. In this particular case it uncovered a bug in your implementation: since the user got this error: https://metacpan.org/source/ILMARI/Catalyst-Model-DBIC-Schema-0.61/lib/Catalyst/Model/DBIC/Schema.pm#L537 then it is obviously DBIC's fault since it is triggerd when your $schema->sources returns an empty list.

But these are details - the main point is that DBIC provides an API with certain guarantees. Lazy-loading is not allowed to break these guarantees unless absolutely justified. It would help greatly if you can do some of the triaging of problems that pop up for users of the alpha version. On the other hand your implied dismissal of a problem earlier in this thread as "it's out of DBIC's hands" was not at all constructive, which is why I had to reply in less uncertain terms ;)

Cheers

@jhthorsen
Copy link
Author

I will try to fix that then.

@jhthorsen
Copy link
Author

Ooops! Forgot to run the test suite. Ignore that last commit.

@ribasushi
Copy link
Collaborator

If you rebase your work on current master you will get useful feedback from travis-ci (updated .travis.yml and friends)

  All tests successful.
  Files=94, Tests=20848, 51 wallclock secs
  Result: PASS
@jhthorsen
Copy link
Author

@agranig: This version should play better with C::M::DBIC::Schema. Could you try it out?

@agranig
Copy link

agranig commented Sep 5, 2013

Seems to work, thanks! I'll test more thoroughly.

@jhthorsen
Copy link
Author

Not enough interest.

@jhthorsen jhthorsen closed this Jun 13, 2014
@ribasushi ribasushi reopened this Jun 13, 2014
@ribasushi
Copy link
Collaborator

not enough interest.

This isn't true. I don't have enough interest to push it in and debug parts that are iffy later. I am committed to get this in. I am simply taking my time to do it correctly. Sorry for not being more forthcoming.

@jhthorsen
Copy link
Author

Oh. I misunderstood you then. Sorry about that @ribasushi :(

@ribasushi
Copy link
Collaborator

Well no, the fault is in fact mine. Basically I am trying to "clean up DBIC's act" because while it is awesome in our little echochamber it is actually a "fractal of bad implemenation" if you hold it up on its own. And given that I do not know another way of fixing it besides... fixing it, I am holding off on adding features where I know of gaping holes in the underpinnings.

While I've been busy with this, I fell behind on communicating what is actually happening. So yes - mea culpa. My plan is to put the above in a more solid policy-like document before the next DBIC release, which in turn will happen before YAPC::NA.

So yes... progress is being made, it's just slow.

@jhthorsen
Copy link
Author

Cool 👍

@gbjk
Copy link
Contributor

gbjk commented Sep 7, 2014

I've rebased this onto the latest release, fixed it, and also fixed everything vivifying on connect.
I'm not saying it's a done deal, but I'll work on testing it, etc.

Either way, that's in a new pull request,so I guess this can be closed of in lieu of that.

@ribasushi
Copy link
Collaborator

Since @gbjk picked up the work where it left off (strictly for his internal use for the time being), and opened a new PR to track the work #59, I am going to close this PR as a duplicate.

Please note - integration into mainline still isn't due until Oct-ish, as other work in the same space needs to be synchronized with this feature. Sorry for the delay, quality software is hard ;)

@ribasushi ribasushi closed this Sep 7, 2014
@jhthorsen jhthorsen mentioned this pull request Sep 7, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants