Bug or breaking change? Model class must now implement `.primary_key`

[bensheldon]

I noticed a recent change now requires any class that has include GlobalID::Identification must now also respond to .primary_key. I think it came from #163

I'm not sure if GlobalId is intended to be used on only Active Record / Active Model, but that's what I've been doing. The readme says:

Mix GlobalID::Identification into any model with a #find(id) class method

If model in this sense means generically "a domain model" than I think this is a bug or breaking change. And if not, the readme needs to be updated to say both .find and .primary_key.

[ghiculescu]

Just saw this, made this PR: #168

[kyrofa]

Just hit this myself. In my book, this is either a bug requiring this fix, or a breaking change requiring a major version bump.

[nvasilevski]

Hi folks, initially this was an intentional breaking change in order to make sure primary_key allows to validate the shape of the model_id and avoid passing more than one value to find() which may lead to more records to be loaded from the database. Also in the initial implementation primary_key was used inside URI::GID to validate the model_id part of the URI as early as possible but we moved away from it in favor of validating inside the default locator

But @ghiculescu preserves the model_id_is_valid? validation so there is no more reason to mandate primary_key interface. Sorry for the disturbance

[kyrofa]

Thanks for the quick fix, folks.

Hi folks, initially this was an intentional breaking change [...]

@nvasilevski just to confirm, are you saying you forgot to bump the major version, or are you saying breaking changes in a minor version bump is to be expected from globalid? I don't mean to pass judgement, I just need to understand the versioning scheme of one of my dependencies. Basically: can I continue using a pessimistic operator, or should I be pinning globalid?

[nvasilevski]

are you saying you forgot to bump the major version

So since release management is a core team's responsibility I couldn't tell whether a breaking change was supposed to be released under a major version. But I wanted to clarify that it was a deliberate decision to require primary_key and we knew about non-AR models being broken since basically all models in tests had to implement primary_key. It wasn't the most optimal decision at the end of the day and I'm glad we end up with a solution to keep backwards compatibility

[kyrofa]

@rafaelfranca would you mind speaking to the versioning strategy, here?

[rafaelfranca]

There isn't any breaking change. primary_key method isn't mandatory in the objects.

[rafaelfranca]

But if there was a intentional breaking change I'd be bumping the major version, not the minor.

[kyrofa]

But if there was a intentional breaking change I'd be bumping the major version, not the minor.

I think this means that not bumping the major version was a mistake. Which is just fine, mistakes happen. But then:

There isn't any breaking change. primary_key method isn't mandatory in the objects.

This makes me doubt. Do you not look at what happened as a breaking change?

Haha, I'm a smidge confused now. @rafaelfranca, to clarify, v1.2.0 made primary_key mandatory, when it wasn't mandatory on v1.1.0. According to @nvasilevski above, that was an intentional breaking change. You're correct, that was fixed in #168 (released in v1.2.1), but this DID break. I just want to have the right expectations in terms of versioning, that's all.