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

Add support for refreshing a single record in a table's IdentityMap #95

Closed
wants to merge 2 commits into from

Conversation

conlanpatrek
Copy link

@conlanpatrek conlanpatrek commented Sep 7, 2018

Opening this up for discussion / inclusion.

Essentially the issue I'm seeing is that if something non Atlas changes a row in the db (separate process, or same process, but arbitrarily executed SQL), there appears to be no way to signal to Atlas to force a refetch of the affected rows. The next time you attempt to read the row, the stale version from the identity map will be returned.

Added a single method to the AbstractTable to allow for manually resetting the in memory row instance to the representation in the database.

@conlanpatrek
Copy link
Author

conlanpatrek commented Sep 7, 2018

A couple of questions that popped into my brain during this:

  1. Should I be running modifySelectedRow after updating the row instance?
  2. If I were to modify this to work with a collection, I would naturally want to run the read query once with all of the primary keys... But what needs to happen to continue to support composite keys in that sense? I would think the query where clause should look something like:
  WHERE (table.prim_col_1 = :bound_1 AND table.prim_col_2 =:bound_2)
  OR (table.prim_col_1 = :bound_3 AND table.prim_col_2=:bound_4)
  // etc...

But I wasn't entirely sure what the right way to go about that sub-grouping, or if that extra work would even be valuable. Thoughts?

@pmjones
Copy link
Contributor

pmjones commented Sep 8, 2018

@conlanpatrek Some other considerations, raised in reference to the 3.x series but perhaps applicable here: atlasphp/Atlas.Mapper#1

@pmjones
Copy link
Contributor

pmjones commented Sep 8, 2018

Should I be running modifySelectedRow after updating the row instance?

That's a good question; I say yes, it should.

@pmjones
Copy link
Contributor

pmjones commented Sep 8, 2018

If I were to modify this to work with a collection

At the table level, there are arrays-of-Rows, but no collections per se.

what needs to happen to continue to support composite keys in that sense?

Cf. the AbstractTable::fetchRows() method for some possible hints in that direction.

@pmjones
Copy link
Contributor

pmjones commented Sep 9, 2018

Ben Bankes notes that Propel uses the word "reload" instead of "refresh." https://twitter.com/benbankes/status/1038781768088932352 Do we feel one is better than the other?

@conlanpatrek
Copy link
Author

I don't know that I have a particular dog in that race, though it seems like 'refetch' keeps the method name in the same vernacular as the rest of atlas

@conlanpatrek
Copy link
Author

Updated based on feedback so far.

  • Renamed method from 'refresh' to 'reload'
  • Added support for reloading an array of rows
  • Added support for reloading an entire IdentityMap of rows.
  • Added modifySelectedRow call after reloading a row.

@pmjones
Copy link
Contributor

pmjones commented Sep 29, 2018

@pokmot Would this approach for the 2.x branch, or something similar, satisfy your needs on atlasphp/Atlas.Mapper#1 ?

@conlanpatrek
Copy link
Author

@pmjones Anything I can do to help this PR along? I don't recall if this is waiting on me for anything.

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.

None yet

2 participants