Patrick Nelson

Nope missed out on that. So much to see and do and in Wellington for only a single night 😭

Patrick Nelson

Photo op with the crew at SilverStripe. Left to right:

Ingo Schommer (sr dev, aka chillu), Sam Minnee (ceo), me, Aaron Carlino (sr dev, aka “Uncle Cheese”) and Serena (m’lady)

Files: Requires logging in with Slack to view/download
Image from iOS
(3) ❤️ (1) (1)
Patrick Nelson

Maybe a better photo of @unclecheese 😄

Files: Requires logging in with Slack to view/download
Image from iOS
Patrick Nelson

and then traced that back to what caused that tangent in the first place at the original ticket above about the unique indexes☝️

Patrick Nelson

ok I think I was able to put a fine point on my concern here: https://github.com/silverstripe/silverstripe-framework/issues/8411#issuecomment-469422545

Show 1 attachment(s)
patricknelson

Potentially stating the obvious after reading everything above, but:

> Modify isInDB() / exists() methods to not just assume a model exists if it's got an ID (either we need to ping the DB or we need to set a flag on loading from DB / writing) - Perhaps replace isInDB() with isPersisted()?

This may also be a good place to mention that ->exists() should point to ->isInDB() (or ->isPersisted() if refactored) on the root DataObject for consistency as well, since ->exists() really does rely on the same underlying logic anyway, regardless of what that implementation ultimately entails. Preferably this would happen in 4.x but I notice this is one of the few places both methods are mentioned together in an open/active issue :smile:

That said, a quick point on that API nomenclature: It's also worth noting that we should probably disambiguate the meaning of ->isInDB() since I think one of the issues tied up in the multi-column index in https://github.com/silverstripe/silverstripe-framework/issues/6819|#6819 (fixed in https://github.com/silverstripe/silverstripe-framework/pull/8831|#8831) was the fact that the ID was always written even with a particular value (e.g. 0 if it was new for that record) which may be fine on the base record but ->isInDB() on a descendant instance $child->isInDB() in memory mid-write operation will return true even though ChildObject's table hasn't actually been written to (and thus not actually entirely in the DB), which makes it confusing.

Maybe this could mean refactoring ->isInDB() (or ->isPersisted()) to determining that a unique ID has been generated, e.g. ->hasUniqueID() or simply ->hasID()? :thinking_face: That would also help folks like me reading the fixes like the one in https://github.com/silverstripe/silverstripe-framework/pull/8831|#8831 understand why that fix actually worked with a slightly more semantically accurate API. I think it would also mesh well with https://github.com/sminnee|@sminnee's point about moving toward Data Mapper / GraphQL friendly data persistence, since in abstract, we're more concerned that we now have a unique ID to work with rather than exactly how it gets persisted (i.e. the more specific but less accurate naming of "Is it in DB yet? Ok then I must have an ID now..." expectation, which may not necessarily hold depending on your underlying DBAL or how you're generating ID's).

Hide attachment content
Patrick Nelson

however, robbie makes a good point in his first comment there about high concurrency and, I think importantly, being able to better handle writes at the app layer

Patrick Nelson

I think fundamentally, even if you move to UUID's, you might still be referencing ->ID to determine if "is in db" or not and, fundamentally, still sorta end up with that same ambiguity. i.e. there are multiple DB records in a single write very easily as soon as you have one object inheriting from another object.

Patrick Nelson

I was just thinking about more deeply exploring the underlying causes of 6819 (i.e. @dhensby’s first point about "Remove race conditions on inserts / ID dependencies" which I believe he mentions in that ticket too, which means I think this RFC should reference the ticket as a case study)

Patrick Nelson

@kinglozzer from what you can tell, is this also only prone to occur when:

  1. previously writing ID on create (old code) and
  2. multi-column indexes and
  3. race conditions (i.e. other PHP process or SQL query busy creating a new row) and/or server-failure mid-execution?

if so, sounds to me like maybe a good solution is also to use transitions as well, but not sure if that's the responsibility of the developer or at the framework level 🤔 or, even better, UUID's. dupe key issue is hopefully completely obliterated, but then that could be too dramatic of a change. (re @dhensby’s insightful comment here actually: https://github.com/silverstripe/silverstripe-framework/issues/6819#issuecomment-404509126). I starting to think of it as a tight binding between DB server and app (php) server. i.e. Both need to know and self-reference some unique ID's, but unfortunately the DB server has to be responsible for asynchronously issue new ID's while at the same time the app server must know those ID's and reference them and then assign them manually elsewhere (a crossing of concerns?) since that ID in the "manipulation" is actually applied across multiple tables which all leverage that same autoincrement ID column (only one of which actually does the work of automatically incrementing). So, I guess the explanation for this fix is then:

  1. Let server's root DataObject table be responsible for centrally/uniquely determining and reserving our new auto-increment ID
  2. Delegate this ID to dependent/inheriting DataObject tables once we have this newly assigned ID.

This is partly why I actually find ->isInDB() confusing. That is actually only true for the base record. So, you are still writing the ID because it's not in the DB 🙂

Show 2 attachment(s)
dhensby

I'd like our IDs to change to be UUIDs so that we don't rely on the DB to assign IDs for us. But this would be a rather large change that would be for 5.x

dhensby

I'd like our IDs to change to be UUIDs so that we don't rely on the DB to assign IDs for us. But this would be a rather large change that would be for 5.x

Hide attachment content