Skip to content

Feature: Introduce sum through many #6

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

Merged
merged 7 commits into from
Jul 11, 2016

Conversation

TSMMark
Copy link
Contributor

@TSMMark TSMMark commented Jul 7, 2016

Like sum cache, but through a join table.

@jeremyevans
Copy link
Owner

Thanks for the patch. While the need for this seems to be rarer than for other supported functions, it does look complete and comes with specs. I'm not too worried about bloat for this library, since it's generally only used during migrations, not during application runtime.

The API for pgt_sum_through_many_cache is a bit too unwieldy, with 9 required arguments. Can you change the API so that it accepts just an options hash, or at least reduces the number of required arguments by moving some of them to options (and having suitable defaults)? Also, could you update the README to document this function?

@TSMMark
Copy link
Contributor Author

TSMMark commented Jul 7, 2016

@jeremyevans I agree 9 required arguments is far too many. Although it will differ from the rest of the API, I can certainly update the method to accept just the options hash. README docs incoming as well!

@TSMMark
Copy link
Contributor Author

TSMMark commented Jul 7, 2016

@jeremyevans I made the changes you suggested. Please take another look.

@jeremyevans
Copy link
Owner

These changes look good. I won't have time to test and merge this until Monday, but one thing I noticed is that this handles inserts and deletes on the child table. I'm not sure if we want to support that. Real foreign keys should be used in the database, so we shouldn't have a case where you can insert a child row that is already referenced by the join table, or delete a child row still referenced by the join table. You still do need to handle updates on the child row, but I think you only need to handle insert, update, and delete on the join table. What are your thoughts on that?

@TSMMark
Copy link
Contributor Author

TSMMark commented Jul 11, 2016

I totally agree with the assertion that real foreign keys should not support this kind of thing. However, I wanted to implement triggers which would handle any case and provide proper sum values regardless of the architecture / FK constraints. Are you concerned about performance? In that case, do you suggest I make the INSERT & DELETE handlers opt-in?

@jeremyevans
Copy link
Owner

Since you've already done the work and it appears to work correctly, I guess there is no point in removing support for INSERT and DELETE on the related table. I'll test this shortly and merge if there are no problems.

@jeremyevans jeremyevans merged commit ab684af into jeremyevans:master Jul 11, 2016
@jeremyevans
Copy link
Owner

I've merged this with some changes, mostly requiring additional constraints so that the trigger functions could be simplified. Could you review and let me know if you are happy with the changes? If so, I'll release a new gem.

@TSMMark TSMMark deleted the feature/sum-through-many branch July 11, 2016 19:09
@TSMMark
Copy link
Contributor Author

TSMMark commented Jul 11, 2016

I suppose it is rather unrealistic to expect anyone to reference columns other than primary keys from a join table. I'm happy with the changes, thanks for checking.

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

Successfully merging this pull request may close these issues.

2 participants