Skip to content

Conversation

@sevenseacat
Copy link
Contributor

This is necessary when running bulk create actions with the sorted?: true option, and also retains consistency with other built-in data layers

Related to ash-project/ash#2452 and 4b6463b

Contributor checklist

Leave anything that you believe does not apply unchecked.

  • I accept the AI Policy, or AI was not used in the creation of this PR.
  • Bug fixes include regression tests
  • Chores
  • Documentation changes
  • Features include unit/acceptance tests
  • Refactoring
  • Update dependencies

This is necessary when running bulk create actions with the `sorted?: true`
option, and also retains consistency with other built-in data layers

Related to ash-project/ash#2452
@zachdaniel
Copy link
Contributor

Ash core should be adding the index from the ref that is set. So moving forward, data layers should only be required to add the ref. If that isn't happening somewhere then I believe its a bug in ash, not here.

@zachdaniel
Copy link
Contributor

Have you tested the issue that you're having against the latest ash version?

@sevenseacat
Copy link
Contributor Author

I have, and it works, but I wasn't entirely confident with your 'I haven't reproduced it but I tried something maybe it will work?' fix there, and the commit that removed bulk_create_index here looks like it was done accidentally.

If the official idea is that data layers don't set bulk_create_index, that's fine, I can fix all the other data layers to also do that (currently ash_postgres is the only one that doesn't).

@zachdaniel
Copy link
Contributor

I have, and it works, but I wasn't entirely confident with your 'I haven't reproduced it but I tried something maybe it will work?' fix there, and the commit that removed bulk_create_index here looks like it was done accidentally.

Currently there is backwards compatibility logic in ash core that will handle setting either of them (except it had a bug), so its okay if data layers don't yet switch to setting ref vs setting index.

It was included in that commit accidentally yes, but the change itself I meant to make.

As for 'I haven't reproduced it but I tried something maybe it will work?', I'm on very limited time and that was the best I had to offer, it was either that or nothing at all. A PR with a test reproducing the original issue is welcome, and would also then help verify the fix 😄

@zachdaniel zachdaniel closed this Dec 6, 2025
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.

3 participants