Skip to content

Conversation

@Ridhwana
Copy link
Contributor

Motivation / Background

This Pull Request has been created to imrove the Active Record Query Interface Guide

Detail

  • Does the example need join_table in the has_and_belongs_to_many definition? -> Probbaly not.
    I kept it as is because I like that we show a real world holistic example
  • Scope out_of_print_and_expensive could be built in terms of out_of_print.costs_more_than(500) (and moved to last?) -> I think it's used as an example of chaining scopes. We don't need it in the initial example I think.
  • The AR Basics guide has a note about SQL knowledge and a couple of links to learn more, maybe a similar note would be useful here as this guide talks a lot about SQL. -> +1
  • There's opportunity to link to composite primary keys guide, or whenever we mention a method that's not been explained (like order) we can link to that within the guide too (we do it for default_scope for instance) -> Maybe we can link to other guides in the beginning? So searching for "composite" or "transaction" would at least find a result with a link to those other guides? Some of the interleaved examples of composite keys might be better kept in the "Composite Primary Guide" I think. The "Query Interface" guide is already pretty overwhelming.
  • There's a section on conditions with strings that warns about SQL Injection, could link to the security guide on the topic. (which we do link a in the subsequent section on array conditions, maybe all of that can be slightly simplified) -> +1
  • Hash range conditions could use methods like all_day and beginning_of_day to simplify the samples. -> +1
    NOTE: I kept it as is because it avoids timezone confusion. Date.yesterday.all_day is evaluated in UTC by default, which can cause subtle differences between local and UTC times, especially if a developer’s config.time_zone is set to something else (e.g., "America/New_York" or "Pretoria"). Time.now.midnight (or, even better, Time.zone.now.midnight) makes the range clearly time-zone aware
  • Samples that show boolean queries with 0 or 1 should likely be changed to false / true instead. (it's simpler to understand, independently of the underlying db) -> +1
  • The examples where it uses created_at as a "date" are not entirely right, especially with grouping, because it will group by timestamp. (not sure how we can show it in an agnostic way, since converting timestamp -> date is usually db specific) -> Maybe group on status instead, like the other examples? grouped by status and customer_id
  • The guide explains only, but there's also except now. (the reverse of only) -> +1
  • reorder explicitly mentions overriding default scope, but it's actually any previously defined order, that can be made clearer -> +1
  • lock and with_lock could be linked to the API. -> +1
  • Mentions references in the includes section, could link to the API. -> +1
  • The query sample for eager_load seems odd, I don't think it'd run two queries like that. -> I have an open PR for that: Fix query result of eager_load example in the guides. [ci-skip] #51940
  • Under scopes arguments & conditionals, we show examples of scopes & class methods being similar, and note at the end one important caveat. It might be a good place to show an example that returning self in a class method would essentially work like a scope. -> +1
  • Duplicated default_scope example after the note. -> I think that example is part of the note, as it ends with a colon: "...applied while updating a record. E.g.:"
  • "The Active Record pattern implements Method Chaining" -> I think "pattern" is wrong here. -> Maybe rename it to "Fluent Interface": https://martinfowler.com/bliki/FluentInterface.html. edit: Ah, I misread this. 'Active Record pattern' is unrelated to Method Chaining indeed.
  • Under Find or Build a New Object, there's good examples of find_or_create_by(!). There's a more recent create_or_find_by(!) that we could expand on the guide. There's also one caveat with the find_or_create method that it's not atomic, so race conditions could potentially create two record. -> +1
  • Food for thought: locking on its own could be an interesting topic for a new guide. It's less about querying, it's kind of a somewhat related but separate concept. The current content is simple / very bare, could likely be expanded with more intricate examples." Maybe even something about transactions & locking together. I added Active Model Transactions & Locking - Documentation Updates to discuss it further, should we decide to go ahead with a new guide.
    NOTE: deferred
  • The first tip might be better placed under the examples? Now it obfuscates the line above it: "Code examples throughout this guide will refer to one or more of the following models:"
  • Also the first bookstore example could probably use a heading or maybe be moved under "Retrieving Object from the Database", as the example doesn't seem to belong to "What is the Active Record Query Interface?"?
  • I don't think create_with is a finder method, so we can probably remove it from the list.
  • "Retrieving Objects from the Database" seems to miss "Retrieving Multiple Objects". Should we add it with an example of Customer.all before the "Retrieving Multiple Objects in batches" section? Or maybe mention where and all the other sections later on ("Conditions", "Ordering", "Limit", "Grouping", etc)?
  • Maybe rename "Conditions" to the less ambiguous "Where Conditions" or "Filtering Results" or "Filtering Results with Where"?
  • Maybe rename "Ordering" to the less ambiguous "Ordering Results"?
  • The distinct section seems somewhat out-of-place in the "Selecting Specific Fields" section. Maybe move it to "Limit and Offset" and rename that section to "Limiting Results"?
  • In the "Joining Tables" section, the "in English" descriptions are inconsistent with emphasizing and.
  • "Overriding Conditions" describes more that overriding where conditions. Not sure what a better name would be though. "Overriding Clauses", "Overriding SQL Clauses"?
  • Moving locking and transactions to a separate guide seem like a good idea to me. Maybe that could also include the "Read Only" section?NOTE: tackle as a separate follow up task
  • Maybe we could move the "Eager Loading" and "Running Explain" section to an "Active Record Performance" guide?
  • Maybe move the "Dynamic Finders" section to "Retrieving a Single Object" after the examples of find_by?
  • In the "Understanding Method Chaining" section should we add a NOTE about "how in the console queries are sometimes unexpectedly executed because it will call inspect on the result"?
  • The "Find or Build a New Object" section should mention create_or_find_by as well.
  • The "Finding by SQL" section might have a better name as it also describes pluck and ids. Or maybe move the pluck, pick and ids methods to the "Retrieving Objects from the Database" section under a new "Retrieving Columns" section?
  • Also the find_by_sql should have it's own heading, like the other methods have.
  • We probably need to mention Async methods like:
    https://edgeapi.rubyonrails.org/classes/ActiveRecord/Calculations.html#method-i-async_pluck
    NOTE: tackle as a separate follow up task, maybe those could be added to an Advanced guide?

Additional information

  • Running EXPLAIN and eager loading can probably be moved to a Performance Guide as suggested. I did not attempt to do this as part of this task, but I will create a separate Basecamp task so that we can determine what else we'd move into this guide and how to give a full view of performance. Thereafter, we can extract it.

  • As per Carlos's and Petrik's suggestions, I agree that we can have a separate guide Active Model Transactions & Locking. However, I'd prefer to tackle this separately. Once we tackle this task, I'll move the content from here and other places to this new guide.

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Unrelated changes should be opened in separate PRs.
  • Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
  • Tests are added or updated if you fix a bug or add a feature.
  • CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

…ot confusing with the examples ebing out of the NOTE
@AmandaPerino AmandaPerino added the rails foundation Rails Foundation PRs label Dec 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants