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

Use locate_many to avoid N+1 #80

Merged
merged 1 commit into from
Feb 12, 2021
Merged

Use locate_many to avoid N+1 #80

merged 1 commit into from
Feb 12, 2021

Conversation

julianrubisch
Copy link
Contributor

Enhancement

Description

To make room for the redesigned functionality, I added another resolver, Resolver::Resources.

This resolver will partition the incoming resource definitions into those that have an SGID and those that don't, and treat them differently.

Because I was referring to a data clump of signed_params, sgids etc. all the time, I wrapped them into a new class ResourceDefinition. It's capable of returning a selector and a controller.

I left the resolution of a renderer outside this class and put it into a renderer_for method, because it needs a connection attribute to function, which would have blurred the boundaries if it went into ResourceDefinition.

It's a bit weird to have it in Resolver::Resources in the first place, but I think I can live with that until a further refactoring.

Closes #78

Why should this be added

The core value of this is a performance improvement by using locate_many, which will avoid N+1 queries in the event that more than one element with an SGID attached should be present.

Checklist

  • My code follows the style guidelines of this project
  • Checks (StandardRB & Prettier-Standard) are passing

@julianrubisch
Copy link
Contributor Author

@rickychilcott would you do me the honor to take a look at this?

@julianrubisch julianrubisch changed the title Extract Resource Resolver #78 Use locate_many to avoid N+1 Feb 9, 2021
@rickychilcott
Copy link
Contributor

I took a look and it looks very reasonable. My only comment is that as we expand the API to add more and more options... extracting the ResourceDefinition into its own dedicated resolver to simplify each piece of the processing of requests coming in. ResourceDefinition being a private class (and utilizing comments to explain the structure) leads me to believe it should be extracted.

Not needed right now unless you agree, but maybe in the future.

👍

@julianrubisch
Copy link
Contributor Author

I know what you’re after but I think I’ll merge this and defer the refactor until we actually need it!

@julianrubisch julianrubisch merged commit 8c5c716 into master Feb 12, 2021
@julianrubisch julianrubisch deleted the 78-locate-many branch February 12, 2021 17:35
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.

Use locate_many to avoid N+1
2 participants