Skip to content

Feature/relationship insert at index sparse#113

Open
tejaede wants to merge 4 commits intomainfrom
feature/relationship-insert-at-index-sparse
Open

Feature/relationship insert at index sparse#113
tejaede wants to merge 4 commits intomainfrom
feature/relationship-insert-at-index-sparse

Conversation

@tejaede
Copy link
Collaborator

@tejaede tejaede commented Feb 24, 2026

This PR works with PhrontHQ/postgre-s-q-l.mod#7

It is intended to add 2 features.

  1. Support add/remove/replace at index for arrays passed to the worker.
  2. Support sparse arrays on rawData

@tejaede tejaede requested a review from marchant February 24, 2026 03:47
@tejaede
Copy link
Collaborator Author

tejaede commented Feb 24, 2026

@marchant

The null items were removed from the array for a couple of reasons. I addressed both, but would like your feedback on the solutions.

1. addedValues / removedValues are converted to Sets which removes an instance of Null. E.g.

let set = new Set([aRole, null, null]);
// set is {aRole, null}

I addressed that here: https://github.com/PhrontHQ/mod/blob/feature/relationship-insert-at-index-sparse/data/service/data-service.js#L4762-L4763

Note that addedValuesSet isn't used anywhere at the moment, but I thought we may still want a Set available to check for intersection.

2. The converter explicitly prevented sparse arrays. I changed it to pass them through, but this happens at a pretty low level so I'm concerned that the change will break other relationships. That didn't show up in my testing, but would appreciate your thoughts.

That new code is here: https://github.com/PhrontHQ/mod/blob/feature/relationship-insert-at-index-sparse/data/converter/raw-foreign-value-to-object-converter.js#L882-L888

Copy link
Contributor

@marchant marchant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my comments

} else {
//TODO: Is this safe? It was added because, in some cases, you may want to preserve a sparse array. E.g. The
//array has a fixed length, but not all values are defined
result.push(value);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The place it could impact would be a toMany that doesn't uses an array column, we should check PG service for that use case of an update. It might already be just looping over and ignoring it, but not certain it does.

Another thing we could do, is add this behavior only if the propertyDescriptor involved has isOrdered == true. But making sure holes in arrays passed to the PG service are ignored when an array column is not involved brings peace of mind and resilience in the face of a mapping / modeling inconsistency

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants