Getting shallow data instead of nested data

bcolthurst commented 5 years ago

I had been looking for a way to just get shallow results (i.e. not the eagerly loaded results that contain nested children objects), and the only way I could figure out to do it was to add a "shallow" flag in the list and read controllers that, if true, skips the includes. Thought this might be useful to others.

tommybananas commented 5 years ago

It looks good to me, let me look into these node 8 test failures and then we'll also want to add documentation to the readme. Thanks for this

bcolthurst commented 5 years ago

in other news, I've since updated my fork with 2 things:

feature: in list queries, instead of destroying the association includes completely, I built a system to, via a query param, allow for selective includes based on the "as" value.
sequelize bug workaround: I noticed that the content-range response header for lists was reflecting the full join row count when doing a list query when associations were included, instead of the count of top-level instances. Traced it to this sequelize bug with findAndCountAll: https://github.com/sequelize/sequelize/issues/4042 -- looks like a bug that was squashed but has come back to life in sequelize. I put in the recommended workaround: { distinct: true} as option to findAndCountAll

fork commit (still has console logs, no doc updates, no tests.....): https://github.com/bcolthurst/finale/commit/f0fb1a225c84f3f06095f3130907ae392edc57da

lmk if these are interesting and I'm happy to help with cleaning it up for a PR or separate PRs (since they're separate issues). Thanks for finale!! -Brendan

tommybananas commented 5 years ago

I'm fine with it being 1 PR or separated, but I'm hoping we can add a quick line or two for documentation before I merge. Also a test case or two would be great if you are so inclined :).

Thanks for your hard work!

tommybananas commented 5 years ago

Can you try merging master? I think the build is failing and a fix is on master. thanks!

bcolthurst commented 5 years ago

Hey -- thanks so much -- I just merged master from finale, removed console logs, added some comment documentation, added 2 sections to the readme. LMK if I need to do anything to update the PR, but I think those commits I made to my fork master are showing up here.

shamoons commented 5 years ago

Any updates on this?

bcolthurst commented 5 years ago

@shamoons hey -- I just merged in the latest tommybananas/finale into my fork and did a number of things. Should have kept it cleaner, but here's what I've done:

merged latest master into my fork
added a Dockerfile for running tests (I found that getting Dtrace to work on my mac didn't go so well, but in a docker container Dtrace works fine)
upgraded a lot of libraries to try and get rid of npm package high severity warnings.
Tried out sequelize 5.x but it has a fatal flaw in how it deals with underscored: true, which broke a ton of tests. So reverted to latest 4.x sequelize
Got the tests passing.

I don't think it's ready for prime-time just yet --- not sure if everyone else wants to upgrade the various dependencies or not.

But I hope to write some tests specific to the "shallow" read/list feature and an "add_to_all" context feature that lets you inject attributes on creation.

tommybananas commented 5 years ago

If you can write those tests and get cleaned up I will merge this. I'm going to work on upgrading the dependencies in the meantime.

tommybananas commented 5 years ago

I just fixed the high severity dependencies. If you could merge in the lastest from master and get everything cleaned up, I'll merge this PR. Thanks!

bcolthurst commented 5 years ago

Hey @tommybananas -- I just added tests for the context.shallow and 'children' query param for selecting which associations to include. Also merged in latest from tommybananas/finale master branch. Had to fix up a dependency on node 5 I errantly introduced. The only thing missing IMHO right now are tests for the context.add_to_all feature I added on the create action. Will work on that now.

bcolthurst commented 5 years ago

Just added in add_to_children functionality, tests, and readme documentation. It's limited to 1-level of nesting -- it doesn't recursively decorate nested objects. This limitation is included in the documentation.

tommybananas commented 5 years ago

Nice work thanks, merged

bcolthurst commented 5 years ago

Nice! Thanks so much!

tommybananas / finale

Getting shallow data instead of nested data #36