AAP-17690 Inventory variables sourced from git project are not getting deleted after being removed from source

awx/api/serializers.py

@@ -116,6 +116,7 @@
 from awx.main.utils.filters import SmartFilter
 from awx.main.utils.plugins import load_combined_inventory_source_options
 from awx.main.utils.named_url_graph import reset_counters
+from awx.main.utils.inventory_vars import update_group_variables
 from awx.main.scheduler.task_manager_models import TaskManagerModels
 from awx.main.redact import UriCleaner, REPLACE_STR
 from awx.main.signals import update_inventory_computed_fields
@@ -1614,8 +1615,66 @@ def validate(self, attrs):
 
         if kind == 'smart' and not host_filter:
             raise serializers.ValidationError({'host_filter': _('Smart inventories must specify host_filter')})
+
         return super(InventorySerializer, self).validate(attrs)
 
+    @staticmethod
+    def _update_variables(variables, inventory_id):
+        """
+        Update the inventory variables of the 'all'-group.
+
+        The variables field contains vars from the inventory dialog, hence
+        representing the "all"-group variables.
+
+        Since this is not an update from an inventory source, we update the
+        variables when the inventory details form is saved.
+
+        A user edit on the inventory variables is considered a reset of the
+        variables update history. Particularly if the user removes a variable by
+        editing the inventory variables field, the variable is not supposed to
+        reappear with a value from a previous inventory source update.
+
+        We achieve this by forcing `overwrite=True` on such an update.
+
+        As a side-effect, variables which have been set by source updates and
+        have survived a user-edit (i.e. they have not been deleted from the
+        variables field) will be assumed to originate from the user edit and are
+        thus no longer deleted from the inventory when they are removed from
+        their original source!
+
+        Note that we use the inventory source id -1 for user-edit updates
+        because a regular inventory source cannot have an id of -1 since
+        PostgreSQL assigns pk's starting from 1 (if this assumption doesn't hold
+        true, we have to assign another special value for invsrc_id).
+
+        :param str variables: The variables as plain text in yaml or json
+            format.
+        :param int inventory_id: The primary key of the related inventory
+            object.
+        """
+        variables_dict = parse_yaml_or_json(variables, silent_failure=False)
+        logger.debug(f"InventorySerializer._update_variables: {inventory_id=} {variables_dict=}")
+        update_group_variables(
+            group_id=None,  # `None` denotes the 'all' group (which doesn't have a pk).
+            newvars=variables_dict,
+            dbvars=None,
+            invsrc_id=-1,
+            inventory_id=inventory_id,
+            overwrite=True,
+        )
+
+    def create(self, validated_data):
+        """Called when a new inventory has to be created."""
+        obj = super().create(validated_data)
+        self._update_variables(validated_data.get("variables") or "", obj.id)
+        return obj
+
+    def update(self, obj, validated_data):
+        """Called when an existing inventory is updated."""
+        obj = super().update(obj, validated_data)
+        self._update_variables(validated_data.get("variables") or "", obj.id)
+        return obj
+
 
 class ConstructedFieldMixin(serializers.Field):
     def get_attribute(self, instance):
@@ -1905,10 +1964,12 @@ def get_related(self, obj):
         return res
 
     def validate(self, attrs):
+        # Do not allow the group name to conflict with an existing host name.
         name = force_str(attrs.get('name', self.instance and self.instance.name or ''))
         inventory = attrs.get('inventory', self.instance and self.instance.inventory or '')
         if Host.objects.filter(name=name, inventory=inventory).exists():
             raise serializers.ValidationError(_('A Host with that name already exists.'))
+        #
         return super(GroupSerializer, self).validate(attrs)
 
     def validate_name(self, value):

awx/main/management/commands/inventory_import.py

@@ -33,6 +33,7 @@
 from awx.main.models.rbac import batch_role_ancestor_rebuilding
 from awx.main.utils import ignore_inventory_computed_fields, get_licenser
 from awx.main.utils.execution_environments import get_default_execution_environment
+from awx.main.utils.inventory_vars import update_group_variables
 from awx.main.signals import disable_activity_stream
 from awx.main.constants import STANDARD_INVENTORY_UPDATE_ENV
 
@@ -457,19 +458,19 @@ def _update_inventory(self):
         """
         Update inventory variables from "all" group.
         """
-        # TODO: We disable variable overwrite here in case user-defined inventory variables get
-        # mangled. But we still need to figure out a better way of processing multiple inventory
-        # update variables mixing with each other.
-        # issue for this: https://github.com/ansible/awx/issues/11623
-
         if self.inventory.kind == 'constructed' and self.inventory_source.overwrite_vars:
             # NOTE: we had to add a exception case to not merge variables
             # to make constructed inventory coherent
             db_variables = self.all_group.variables
         else:
-            db_variables = self.inventory.variables_dict
-            db_variables.update(self.all_group.variables)
-
+            db_variables = update_group_variables(
+                group_id=None,  # `None` denotes the 'all' group (which doesn't have a pk).
+                newvars=self.all_group.variables,
+                dbvars=self.inventory.variables_dict,
+                invsrc_id=self.inventory_source.id,
+                inventory_id=self.inventory.id,
+                overwrite=self.overwrite_vars,
+            )
         if db_variables != self.inventory.variables_dict:
             self.inventory.variables = json.dumps(db_variables)
             self.inventory.save(update_fields=['variables'])

awx/main/migrations/0203_inventorygroupvariableswithhistory_and_more.py

@@ -0,0 +1,32 @@
+# Generated by Django 4.2.20 on 2025-04-24 09:08
+
+from django.db import migrations, models
+import django.db.models.deletion
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('main', '0202_delete_token_cleanup_job'),
+    ]
+
+    operations = [
+        migrations.CreateModel(
+            name='InventoryGroupVariablesWithHistory',
+            fields=[
+                ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
+                ('variables', models.JSONField()),
+                ('group', models.ForeignKey(null=True, on_delete=django.db.models.deletion.CASCADE, related_name='inventory_group_variables', to='main.group')),
+                (
+                    'inventory',
+                    models.ForeignKey(null=True, on_delete=django.db.models.deletion.CASCADE, related_name='inventory_group_variables', to='main.inventory'),
+                ),
+            ],
+        ),
+        migrations.AddConstraint(
+            model_name='inventorygroupvariableswithhistory',
+            constraint=models.UniqueConstraint(
+                fields=('inventory', 'group'), name='unique_inventory_group', violation_error_message='Inventory/Group combination must be unique.'
+            ),
+        ),
+    ]

awx/main/models/__init__.py

@@ -33,6 +33,7 @@
     InventorySource,
     InventoryUpdate,
     SmartInventoryMembership,
+    InventoryGroupVariablesWithHistory,
 )
 from awx.main.models.jobs import (  # noqa
     Job,

awx/main/models/inventory.py

@@ -1402,3 +1402,38 @@ class Meta:
 
     def get_absolute_url(self, request=None):
         return reverse('api:inventory_script_detail', kwargs={'pk': self.pk}, request=request)
+
+
+class InventoryGroupVariablesWithHistory(models.Model):
+    """
+    Represents the inventory variables of one inventory group.
+
+    The purpose of this model is to persist the update history of the group
+    variables. The update history is maintained in another class
+    (`InventoryGroupVariables`), this class here is just a container for the
+    database storage.
+    """
+
+    class Meta:
+        constraints = [
+            # Do not allow the same inventory/group combination more than once.
+            models.UniqueConstraint(
+                fields=["inventory", "group"],
+                name="unique_inventory_group",
+                violation_error_message=_("Inventory/Group combination must be unique."),
+            ),
+        ]
+
+    inventory = models.ForeignKey(
+        'Inventory',
+        related_name='inventory_group_variables',
+        null=True,
+        on_delete=models.CASCADE,
+    )
+    group = models.ForeignKey(  # `None` denotes the 'all'-group.
+        'Group',
+        related_name='inventory_group_variables',
+        null=True,
+        on_delete=models.CASCADE,
+    )
+    variables = models.JSONField()  # The group variables, including their history.

awx/main/tests/data/projects/inventory_vars/inventory_var_deleted_in_source.ini

@@ -0,0 +1,3 @@
+[all:vars]
+a=value_a
+b=value_b

awx/main/tests/live/tests/test_inventory_vars.py

@@ -0,0 +1,132 @@
+import subprocess
+import time
+
+import pytest
+from unittest import mock
+
+from awx.main.models.projects import Project
+from awx.main.models.organization import Organization
+from awx.main.models.inventory import Group, Inventory, InventoryUpdate, InventorySource
+from awx.main.tests.live.tests.conftest import wait_for_job
+
+
+NAME_PREFIX = "test-ivu"
+GIT_REPO_FOLDER = "inventory_vars"
+
+
+def create_new_by_name(model, **kwargs):
+    """
+    Create a new model instance. Delete an existing instance first.
+
+    :param model: The Django model.
+    :param dict kwargs: The keyword arguments required to create a model
+        instance. Must contain at least `name`.
+    :return: The model instance.
+    """
+    name = kwargs["name"]
+    try:
+        instance = model.objects.get(name=name)
+    except model.DoesNotExist:
+        pass
+    else:
+        print(f"FORCE DELETE {name}")
+        instance.delete()
+    finally:
+        instance = model.objects.create(**kwargs)
+    return instance
+
+
+def wait_for_update(instance, timeout=3.0):
+    """Wait until the last update of *instance* is finished."""
+    start = time.time()
+    while time.time() - start < timeout:
+        if instance.current_job or instance.last_job or instance.last_job_run:
+            break
+        time.sleep(0.2)
+    assert instance.current_job or instance.last_job or instance.last_job_run, f'Instance never updated id={instance.id}'
+    update = instance.current_job or instance.last_job
+    if update:
+        wait_for_job(update)
+
+
+@pytest.fixture
+def organization():
+    name = f"{NAME_PREFIX}-org"
+    instance = create_new_by_name(Organization, name=name, description=f"Description for {name}")
+    yield instance
+    instance.delete()
+
+
+@pytest.fixture
+def project(organization, live_tmp_folder):
+    name = f"{NAME_PREFIX}-project"
+    instance = create_new_by_name(
+        Project,
+        name=name,
+        description=f"Description for {name}",
+        organization=organization,
+        scm_url=f"file://{live_tmp_folder}/{GIT_REPO_FOLDER}",
+        scm_type="git",
+    )
+    yield instance
+    instance.delete()
+
+
+@pytest.fixture
+def inventory(organization):
+    name = f"{NAME_PREFIX}-inventory"
+    instance = create_new_by_name(
+        Inventory,
+        name=name,
+        description=f"Description for {name}",
+        organization=organization,
+    )
+    yield instance
+    instance.delete()
+
+
+@pytest.fixture
+def inventory_source(inventory, project):
+    name = f"{NAME_PREFIX}-invsrc"
+    inv_src = InventorySource(
+        name=name,
+        source_project=project,
+        source="scm",
+        source_path="inventory_var_deleted_in_source.ini",
+        inventory=inventory,
+    )
+    with mock.patch('awx.main.models.unified_jobs.UnifiedJobTemplate.update'):
+        inv_src.save()
+    yield inv_src
+    inv_src.delete()
+
+
+def test_inventory_var_deleted_in_source(live_tmp_folder, project, inventory, inventory_source):
+    """
+    Verify that a variable which is deleted from its (git-)source between two
+    updates is also deleted from the inventory.
+
+    Verifies https://issues.redhat.com/browse/AAP-17690
+    """
+    inventory_source.update()
+    wait_for_update(inventory_source)
+    inv_vars = Inventory.objects.get(name=inventory.name).variables_dict
+    print(f"After 1st update: {inv_vars=}")
+    assert inv_vars == {"a": "value_a", "b": "value_b"}
+    # Remove variable `a` from source.
+    repo_path = f"{live_tmp_folder}/{GIT_REPO_FOLDER}"
+    path = f"{repo_path}/inventory_var_deleted_in_source.ini"
+    with open(path, "w") as fp:
+        fp.write("[all:vars]\n")
+        fp.write("b=value_b\n")
+    subprocess.run('git add .; git commit -m "Update variables"', cwd=repo_path, shell=True)
+    # Update the project to sync the changed repo contents.
+    project.update()
+    wait_for_update(project)
+    # Update the inventory from the changed source.
+    inventory_source.update()
+    wait_for_update(inventory_source)
+    #
+    inv_vars = Inventory.objects.get(name=inventory.name).variables_dict
+    print(f"After 2nd update: {inv_vars=}")
+    assert inv_vars == {"b": "value_b"}