Key value mapping in indexedMetadataFields required
prolic opened this issue · 12 comments
Current implementation uses substr to map _aggregate_id to aggregate_id. Problem is, users are free to index other metadata fields as well, so they can index a field named 'house', but would need to generate a field named 'ouse'. But what if they have another field named 'mouse' that needs to be indexed as well? As you see, the substr solution was not that a good idea.
Unfortunately I released this yesterday and changing again is a BC. I would however still want to change it, because the problem is real and not solvable with current release and also the current release is a BC because of 'house' -> 'ouse' naming needs. And lf course, the release is only one day old....
Thoughts? @codeliner @kochen
I think it's a bugfix and not a BC break if we change it back. Simple question: Why not only remove first sign if it is an underscore?
@prolic yes, you are right, but that it only possible on (any) CustomMariaDBPersistenceStrategy) so for the prooph component itself it's "safe" enough.
We could easily introduce the mapping, just like I did in the first proposed commit of the fix:
f25eefc#diff-ba722b29dfda4a6ee77c2e065683291cR34
But for a more future proof solution, I think we should be able to check/validate that mapping also when the DB Table is created.
So either we roll back to your first commit, or we substr only when first character is '_'. What do you think?
I wouldn't rollback as it's not a full implementation, but we could "fix" it into that way.
I did add a comment in the README about this for CustomStrategy implementer.
So, what I think is better, is to actually leave it as is for now, and think of a proper solution for the next major release, with any potential BC that might occur.
Don't worry about next major release, because it will not have any persistence strategies at all.
the substr solution is really bad, because it denies custom indexes. if we agree to at least only remove first character if it's _, then we can still allow custom indexes.
the thing is that removing only the first _ means any other index won't be use, and won't be possible to even use it.
We might as well implement the mapping and get over with it...
I don't get what you mean, can you give an example?
I can easily change the substr with a mapping, with the assumption that the indexedMetadataFields has a correctly formatted mapping.
public function indexedMetadataFields(): array
{
return [
'_aggregate_id' => 'aggregate_id',
'_aggregate_type' => 'aggregate_itype,
'_aggregate_version' => 'aggregate_version',
];
}
and then in convertToColumn (something like):
$indexedColumns = $this->persistenceStrategy->indexedMetadataFields();
if (in_array($match['field'], array_keys($indexedColumns))) {
$match['field'] = indexedColumns[$match['field']];
$match['fieldType'] = FieldType::MESSAGE_PROPERTY();
}
Shall I just provide an initial PR?
yes please
done