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

Finding Orphan Records #34727

Merged
merged 1 commit into from Jan 11, 2020
Merged

Conversation

@tomrossi7
Copy link
Contributor

tomrossi7 commented Dec 17, 2018

Summary

As discussed here, I would like to introduce a way for simplifying the search for orphan records within ActiveRecord.

Currently you would do something like this:
Post.left_joins(:author).where(authors: { id: nil })

I would like to introduce an API like this:
Post.where.missing(:author)

Other Information

I've gotten started, but could use some feedback and direction from someone more experienced with the innards of ActiveRecord. I've modeled the missing method after the not method in the query_methods.rb:

  def missing(*args)
    args.each do |arg|
      opts = { arg => { id: nil } }
      @scope.left_joins(arg)
      @scope.references!(PredicateBuilder.references(opts))
      @scope.where_clause += @scope.where(opts)
    end
    @scope
  end

1. Adding the left join

In order to add the join to the current scope, I am just calling the left_joins method, but that doesn't appear to actually modify the scope. Should I be looking at the spawn method?

2. References in the where clause

I copied the references! call from the not method, but I'm not sure what it is actually doing. That is where my error is currently pointing. ☹️

@rails-bot rails-bot bot added the activerecord label Dec 17, 2018
@tomrossi7 tomrossi7 force-pushed the higher-pixels:add_missing_to_where_chain branch from bac5890 to 1eeae34 Dec 18, 2018
@filipewl

This comment has been minimized.

Copy link

filipewl commented Apr 10, 2019

@tomrossi7, maybe it's also missing some comments for the method usage, like we have for not:

# Returns a new relation expressing WHERE + NOT condition according to
# the conditions in the arguments.
#
# #not accepts conditions as a string, array, or hash. See QueryMethods#where for
# more details on each format.
#
# User.where.not("name = 'Jon'")
# # SELECT * FROM users WHERE NOT (name = 'Jon')
#
# User.where.not(["name = ?", "Jon"])
# # SELECT * FROM users WHERE NOT (name = 'Jon')
#
# User.where.not(name: "Jon")
# # SELECT * FROM users WHERE name != 'Jon'
#
# User.where.not(name: nil)
# # SELECT * FROM users WHERE name IS NOT NULL
#
# User.where.not(name: %w(Ko1 Nobu))
# # SELECT * FROM users WHERE name NOT IN ('Ko1', 'Nobu')
#
# User.where.not(name: "Jon", role: "admin")
# # SELECT * FROM users WHERE name != 'Jon' AND role != 'admin'

@tomrossi7

This comment has been minimized.

Copy link
Contributor Author

tomrossi7 commented Apr 10, 2019

@filipewl definitely! I was hoping to get it working and then I can write up the usage.

@rafaelfranca

This comment has been minimized.

Copy link
Member

rafaelfranca commented Apr 11, 2019

Why the [ci skip] given this is changing code?

@tomrossi7

This comment has been minimized.

Copy link
Contributor Author

tomrossi7 commented Apr 11, 2019

@rafaelfranca sorry, this is just a strategy and not ready to be tested. I need help understanding how to build the left joins in the query_methods module.

@st0012

This comment has been minimized.

Copy link
Contributor

st0012 commented Apr 20, 2019

@tomrossi7 I still think you should enable to CI. Because failing tests can also provide useful information

@rafaelfranca

This comment has been minimized.

Copy link
Member

rafaelfranca commented May 1, 2019

I have a version of your code with the tests passing on this branch https://github.com/rafaelfranca/omg-rails/tree/add_missing_to_where_chain.

@tomrossi7 tomrossi7 changed the title Finding Orphan Records [ci skip] Finding Orphan Records May 2, 2019
@tomrossi7

This comment has been minimized.

Copy link
Contributor Author

tomrossi7 commented May 2, 2019

Thanks for the help @rafaelfranca !

@jeremy

This comment has been minimized.

Copy link
Member

jeremy commented May 3, 2019

Cool idea. WhereChain reads well here, but it's designed for modifiers on where conditions, like not negating the given conditions. How would this compare as a top-level relation query method, e.g. Post.missing(:author) or Post.authorless?

@tomrossi7

This comment has been minimized.

Copy link
Contributor Author

tomrossi7 commented May 6, 2019

@jeremy Thanks for checking this out! We originally tried to come up with a name for the method that read well, but @dhh recommended using modifiers on #where rather than hanging new methods off the root scope.

@tomrossi7

This comment has been minimized.

Copy link
Contributor Author

tomrossi7 commented May 16, 2019

Any direction on the naming for this and where it should live? I'm happy to write up documentation and make a similar patch for Rails 6.0.

@rails-bot

This comment has been minimized.

Copy link

rails-bot bot commented Dec 17, 2019

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

@rails-bot rails-bot bot added the stale label Dec 17, 2019
@rails-bot rails-bot bot closed this Dec 25, 2019
@dhh dhh reopened this Dec 25, 2019
@rails-bot rails-bot bot removed the stale label Dec 25, 2019
@dhh

This comment has been minimized.

Copy link
Member

dhh commented Dec 25, 2019

@tomrossi7 could you add a change log entry? I’ll merge 👍

@dhh dhh self-assigned this Dec 25, 2019
@tomrossi7

This comment has been minimized.

Copy link
Contributor Author

tomrossi7 commented Dec 30, 2019

@dhh I just added it to the ActiveRecord changelog file. I put some examples in the comments as well. Thanks for checking out my first commit to Rails code!

@dhh

This comment has been minimized.

Copy link
Member

dhh commented Dec 30, 2019

Seems like some build failures?


def setup
super
@name = "title"
end

def test_missing_with_association

This comment has been minimized.

Copy link
@simi

simi Dec 30, 2019

Contributor

What about to introduce more high-level test, not one testing the implementation?

post_without_author = Post.create(author: nil)
assert_equal [post_without_author], Post.without(:author).to_a
@kaspth

This comment has been minimized.

Copy link
Member

kaspth commented Dec 31, 2019

Think the build failures could be fixed by a rebase. Please squash your commits down to 1 too 🙏

@tomrossi7

This comment has been minimized.

Copy link
Contributor Author

tomrossi7 commented Jan 11, 2020

How do these tests look?

    def test_missing_with_association
      assert posts(:authorless).author.blank?
      assert_equal [posts(:authorless)], Post.where.missing(:author).to_a
    end

    def test_missing_with_multiple_association
      assert posts(:authorless).comments.empty?
      assert_equal [posts(:authorless)], Post.where.missing(:author, :comments).to_a
    end

I like that I am not testing the implementation, but not sure how we feel about relying on existing fixtures. For clarity, I started each test with an assertion of the fixture setup, but maybe that isn't necessary? I would hate for fixtures to change in the future and break the test or make the test meaningless.

@tomrossi7 tomrossi7 closed this Jan 11, 2020
@tomrossi7 tomrossi7 force-pushed the higher-pixels:add_missing_to_where_chain branch from 1978764 to b50ce8d Jan 11, 2020
@tomrossi7 tomrossi7 reopened this Jan 11, 2020
@tomrossi7 tomrossi7 force-pushed the higher-pixels:add_missing_to_where_chain branch from c91a989 to 9bfe89e Jan 11, 2020
@kaspth kaspth merged commit 6368e80 into rails:master Jan 11, 2020
1 of 2 checks passed
1 of 2 checks passed
build
Details
buildkite/rails Build #66383 failed (9 minutes, 48 seconds)
Details
@kaspth

This comment has been minimized.

Copy link
Member

kaspth commented Jan 11, 2020

Looks like the build failures are unrelated, thanks so much @tomrossi7 ❤️🙌

@kaspth

This comment has been minimized.

Copy link
Member

kaspth commented Jan 11, 2020

I like that I am not testing the implementation, but not sure how we feel about relying on existing fixtures. For clarity, I started each test with an assertion of the fixture setup, but maybe that isn't necessary? I would hate for fixtures to change in the future and break the test or make the test meaningless.

Relying on existing fixtures is perfectly fine. I probably wouldn't have tested the relationships as the tests would've broken anyway, but just fine for these two cases 😊

yujideveloper added a commit to yujideveloper/activerecord-missing that referenced this pull request Jan 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

10 participants
You can’t perform that action at this time.