Changes to DomainContraint to prevent database call per request #309
Conversation
| module Features | ||
| module DomainHelpers | ||
| def with_domain(host) | ||
| original_host = Capybara.app_host | ||
| Capybara.app_host = "http://#{host}" | ||
| yield | ||
| ensure | ||
| Capybara.app_host = original_host | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
This was a good idea.
- Switches logic to match on non event host domain set in ENV variable rather than website domains stored in DB - Moves config/initializers/domain_constraint.rb to app/constraints folder - Set Rails configuration events_hosts value from EVENTS_HOSTS env variable with fallback defaults for non production environments - Uses lvh.me as domain to more accurately test domain constraint logic in feature specs
jonsgreen
force-pushed
the
flagrant/non-db-domain-match
branch
from
10e789b to
0cda55c
Compare
|
This seems like a fine solution although it moves the pattern-matching out of postgres and into memory, right? This might not be a huge deal if caching is utilized properly and given the low level of traffic. Consider not-fixing this (in spite of my previous concern). |
|
@adarsh Yes, this would change the constraint so that the matching does not depend on a database query on every page request. It is true that caching would eliminate any performance concern for requests to the websites. The main issue would be for the speakers and organizers interacting with the rest of CFP app since that part cfp-app will not be cached. On the other hand, the performance penalty is small and maybe not as critical for those use cases. I will say that another benefit from this PR is that that I think the integration specs for domains would be better implemented using 'lvh.me' since it better reflects what would happen in production. I guess I felt responsible to at least try a different approach since the current one is considered to be a bad practice. I will leave it to you to decide whether to merge this or put it aside for now. |
Reason for Change
Recently while watching this GoRails video on Domain and Subdomain Constraints I noticed that it was recommended to not do database requests in your routes constraints since it means that every request will need to make the request.
It occurred to me that we could avoid the requests entirely by instead matching against the hosts for the events side of cfp-app. It means the environment variable needs to be set for most of cfp app to be accessible but that should not change much once it is done.
Changes
rather than website domains stored in DB
variable with fallback defaults for non production environments
in feature specs