Illegal offset type exception with Composite Keys

[zoul0813]

When a model has a composite key, Illegal offset type exceptions are thrown by third-party packages that rely on Eloquent's getKeyName() to return a string.

For example, Elasticquent depends on getKeyName() to hydrate models retrieved from ElasticSearch.

$key_name = $this->getKeyName();
$attributes = $hit['_source'];
if (isset($hit['_id'])) {
  $attributes[$key_name] = is_numeric($hit['_id']) ? intval($hit['_id']) : $hit['_id'];
}

I recommend reverting getKeyName() and getKey() to return the value of the $primaryKey column in the model, and re-introducing the getCompositeKeyName() and getCompositeKey() methods to allow developers to work with composite keys when they want/need to do so.

The problem is that Eloquent does not support composite keys "out of the box", so a lot of third-party packages use getKeyName() and getKey() to retrieve details about the model and expect the return value to be a string for getKeyName() and just use it as an array index.

[zoul0813]

Ref #112

[baopham]

Maybe can make getKeyName() return partition_id:sort_id instead? I don't want to steer away too much from dynamodb concept.

[zoul0813]

@baopham In my current situation, I have a "messages" table that has a thread_id and id column. The thread_id is the partition key, and the id is the sort key.

All of my data is indexed in ElasticSearch, and my application retrieves data directly from ElasticSearch ... I'm using Elasticquent for this, which allows me to query ElasticSearch and hydrate Eloquent models from the results. The models thread_id and id columns are hydrated properly, but Elasticquent "assumes" that the model has a single primary key (as do many other projects) and attempts to set this value using the ElasticSearch _id (as I may not have indexed the id directly ... in my case, I did ... but Elasticquent doesn't know that nor should it care).

I don't believe laravel-dynamodb should return an array when calling getKeyName() as this is not the expected result since Eloquent does not have support for composite keys. The Eloquent\Model's getKeyName() method says it should return a string.

If we returned partition_id:sort_id this would break packages like Elasticquent as it's attempting to take the _id from ElasticSearch and set the $primaryKey of the model to it ... which would wind up creating something like $model['thread_id:id'] in my case ... which is not a valid column. In my case, I set the $primaryKey of my model to id, and manage the thread_id separately as I'm aware of it's use ... while other packages my project relies on may not be aware of it.

I recommend we stick with Eloquent support as much as possible, and introduce additional methods like getCompositeKeyName() for special use-cases. As long as Elasticquent hydrates my model with the thread_id and id properties, the model works without issue ... as it has for the past few months, until the recent updates last week.

[baopham]

That's a good point. Should be a quick change. For getCompositeKeyName, user can just call $this->compositeKey or $this->getKeyNames()

[baopham]

Can you try dev-master now?

[zoul0813]

I’d recommend using Eloquent style methods, and not just pluralizing as the “s” on the end of getKeyNames is too similar to getKeyName and could cause headaches for people ... especially if they rely on type-ahead in their editor. It’s also not clear “at a glance” as it’s just a plural... getCompositeKeyNames() seems like a clearer name? Along with a getCompositeKeys() method so we aren’t directly accessing the $compositeKey property and so models can override the return for special use cases.

[baopham]

It’s also not clear “at a glance” as it’s just a plural...

Ummm, I think that's not too convincing to me. The idea of getKeyNames is not to get just the composite key. I want a method that will always give me an array of key names even if it's not a composite model. A stronger reason is more for getKeys where I can get a map of key name => key value - all valid for both composite / non-composite.

[zoul0813]

Hrm ... ok, I can see that reasoning.

[zoul0813]

The latest update works, and no longer causes the "Illegal offset type" error in Elasticquent's hydration.

[baopham]

I find having to switch back and forth between array and string depending whether model is composite or not is annoying. So just something that will give me a consistent return type felt more helpful.

So I'm not into having the "composite key" in the function names, but am open to better names for these functions.

[zoul0813]

I still think getKeyNames and getKeyName are too similar, but you're reasoning for not including 'composite' in the method name makes sense ... as you may or may not be working with composite keys. Unfortunately, I don't have any good ideas on better names ... best I can come up with is getKeyNameArray() which seems a bit awkward ...

[baopham]

Naming is hard 😄 Agree that they look so similar... I'll keep this in mind and we can come up with something better in the future.

Closing this issue for now.

[baopham]

Tagged version 4.1.1