Problem/Motivation

Recipes are supposed to be idempotent, safe to apply twice or more 🤷‍♂️. That's the whole reason core actions like entity_create:createIfNotExists exist. This recipe isn't idempotent. Applying it a second time on a site that already has it fails outright.

The direct cause: setupVdbServerWithDefaults (on search_api.server.content_vector) and setupVdbIndex (on search_api.index.content_vector) both call $this->entityTypeManager->getStorage(...)->create($value)->save() unconditionally, with no check for whether the entity already exists. That code lives in the ai module, not in this recipe, so the actual fix might not be something this recipe's maintainers can make on their own.

The real question to settle first is whether idempotency is a goal for this recipe at all. If it is, that's a constraint on which actions it can use, and probably a conversation with the ai module's maintainers about adding an existence check there. If the answer is no, that's a fair call too, but it should be made on purpose, not left as a side effect of which action happened to get picked.

Steps to reproduce

  1. On a fresh site: drush recipe:apply ai_recipe_content_search_vector.
  2. Apply it again: drush recipe:apply ai_recipe_content_search_vector.

Second apply fails:

In SetupVdbServer.php line 116:
  Could not save the configuration.

Proposed resolution

Confirm idempotency is actually wanted here. If it is, raise an existence check with the ai module's maintainers for setupVdbServerWithDefaults and setupVdbIndex, matching what entity_create:createIfNotExists already does. Happy to submit an MR once that's agreed.

Remaining tasks

  • Decide whether this recipe should be idempotent.
  • If yes, coordinate with the ai module's maintainers on adding an existence check to SetupVdbServer::apply() and SetupVdbIndex::apply().
Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

abhisekmazumdar created an issue. See original summary.

abhisekmazumdar’s picture

Okay, this also means that when https://www.drupal.org/project/ai_recipe_answers marks this recipe as a dependency, ai_recipe_content_search_vector will be applied again, which will break the setup. It would be great if we could make ai_recipe_content_search_vector idempotent.

a.dmitriiev’s picture

I also believe this should be fixed in AI Core. But be aware, that as recipe passes some server and index values, probably when checking whether config entities already exist, you also need to check that the provided values from the recipe match the existing values, as normally the AI Server settings can't be changed (embeddings model, size of chunks, vector dimensions) as this will require wiping out the remote index.

abhisekmazumdar’s picture

Assigned: Unassigned » abhisekmazumdar
arianraeesi’s picture

abhisekmazumdar’s picture

Assigned: abhisekmazumdar » Unassigned
Status: Active » Postponed

Filed a companion issue against the ai module: 3586729. That's where the actual fix needs to happen: an existence check in SetupVdbServer::apply() and SetupVdbIndex::apply(), matching what core's entity_create:createIfNotExists does.

So I'm marking it postponed until that happens...

a.dmitriiev’s picture

Status: Postponed » Active

The issue in AI Core was merged and will be added to the next 1.5.x release.

abhisekmazumdar’s picture

Status: Active » Fixed

I verified that the fix from https://git.drupalcode.org/project/ai/-/work_items/3586729 resolves this.

I tested it, and it works. This recipe can be applied twice, which makes it usable as a dependency in other recipes too. That is especially useful for https://www.drupal.org/project/ai_recipe_answers.

The existence checks in SetupVdbServer::apply() and SetupVdbIndex::apply() work. Reapplying this recipe is now safe.

One thing worth deciding

This recipe's composer.json currently requires drupal/ai: ^1.3, so a fresh install can still pull in 1.3.x or 1.4.x, and neither includes the idempotency fix. Anyone installing this recipe on one of those versions will hit the same bug.

I would bump the constraint to drupal/ai: ^1.5 once that release ships.

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

abhisekmazumdar’s picture

Status: Fixed » Reviewed & tested by the community
a.dmitriiev’s picture

Let's pin the version to 1.5.0@RC as this version https://www.drupal.org/project/ai/releases/1.5.0-rc4 already has it included

abhisekmazumdar’s picture

Status: Reviewed & tested by the community » Needs review

Done MR opned.

a.dmitriiev’s picture

Status: Needs review » Needs work

Please check my comment in the MR

abhisekmazumdar’s picture

Status: Needs work » Needs review

Done back to review.

a.dmitriiev’s picture

Status: Needs review » Fixed

Merged!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.