Nope missed out on that. So much to see and do and in Wellington for only a single night 😭
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)
and then traced that back to what caused that tangent in the first place at the original ticket above about the unique indexes☝️
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
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
->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
->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).
i.e. decouple and don't rely on the DB to assign ID's.
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
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.
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)
yeah -- I was just reading up #8411
@kinglozzer from what you can tell, is this also only prone to occur when:
IDon create (old code) and
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:
DataObjecttable be responsible for centrally/uniquely determining and reserving our new auto-increment ID
DataObjecttables 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 🙂
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
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.xHide attachment content