Repository: ansible/awx
File: awx/main/tasks/facts.py (Python)

Code before:

    host_names = summary.get('hosts_cached', [])
    hosts_cached = Host.objects.filter(name__in=host_names).order_by('id').iterator()

    # Path where individual fact files were written
    fact_cache_dir = os.path.join(artifacts_dir, 'fact_cache')

    hosts_to_update = []

    for host in hosts_cached:
        filepath = os.path.join(fact_cache_dir, host.name)
        if not os.path.realpath(filepath).startswith(fact_cache_dir):
            logger.error(f'Invalid path for facts file: {filepath}')
            continue

        if os.path.exists(filepath):
            # If the file changed since we wrote the last facts file, pre-playbook run...
            modified = os.path.getmtime(filepath)
            if not facts_write_time or modified >= facts_write_time:
                try:
                    with codecs.open(filepath, 'r', encoding='utf-8') as f:
                        ansible_facts = json.load(f)
                except ValueError:
                    continue

                host.ansible_facts = ansible_facts
                host.ansible_facts_modified = now()
                hosts_to_update.append(host)
                logger.info(
                    f'New fact for inventory {smart_str(host.inventory.name)} host {smart_str(host.name)}',
                    extra=dict(
                        inventory_id=host.inventory.id,
                        host_name=host.name,
                        ansible_facts=host.ansible_facts,
                        ansible_facts_modified=host.ansible_facts_modified.isoformat(),
                        job_id=job_id,
                    ),
                )
                log_data['updated_ct'] += 1
            else:
                log_data['unmodified_ct'] += 1
        else:
            # if the file goes missing, ansible removed it (likely via clear_facts)
            # if the file goes missing, but the host has not started facts, then we should not clear the facts
            host.ansible_facts = {}
            host.ansible_facts_modified = now()
            hosts_to_update.append(host)
            logger.info(f'Facts cleared for inventory {smart_str(host.inventory.name)} host {smart_str(host.name)}')
            log_data['cleared_ct'] += 1

        if len(hosts_to_update) >= 100:

Review comment (suggestion):
I'm only slightly concerned that `except ValueError` was intended to catch an error in this line. Specifically, that `ansible_facts` might contain a type not compatible with the field. Yet, this would have to be a JSON type. So... like an integer or a dictionary might raise a ValueError? If that does happen, then we would prefer to continue the loop as opposed to error the method.

But I don't feel strongly about this at the current moment, because if it's a concern, then it should have a test. However, as a code-style point, I think the exception is unlikely to be meaningful in its current location.

Code after:
    host_names = summary.get('hosts_cached', [])
    hosts_cached = Host.objects.filter(name__in=host_names).order_by('id').iterator()
    # Path where individual fact files were written
    fact_cache_dir = os.path.join(artifacts_dir, 'fact_cache')
    hosts_to_update = []

    for host in hosts_cached:
        filepath = os.path.join(fact_cache_dir, host.name)
        if not os.path.realpath(filepath).startswith(fact_cache_dir):
            logger.error(f'Invalid path for facts file: {filepath}')
            continue

        if os.path.exists(filepath):
            # If the file changed since we wrote the last facts file, pre-playbook run...
            modified = os.path.getmtime(filepath)
            if not facts_write_time or modified >= facts_write_time:
                try:
                    with codecs.open(filepath, 'r', encoding='utf-8') as f:
                        ansible_facts = json.load(f)
                except ValueError:
                    continue

                if ansible_facts != host.ansible_facts:
                    host.ansible_facts = ansible_facts
                    host.ansible_facts_modified = now()
                    hosts_to_update.append(host)
                    logger.info(
                        f'New fact for inventory {smart_str(host.inventory.name)} host {smart_str(host.name)}',
                        extra=dict(
                            inventory_id=host.inventory.id,
                            host_name=host.name,
                            ansible_facts=host.ansible_facts,
                            ansible_facts_modified=host.ansible_facts_modified.isoformat(),
                            job_id=job_id,
                        ),
                    )
                    log_data['updated_ct'] += 1
                else:
                    log_data['unmodified_ct'] += 1
            else:
                log_data['unmodified_ct'] += 1
        else:
            # if the file goes missing, ansible removed it (likely via clear_facts)
            # if the file goes missing, but the host has not started facts, then we should not clear the facts
            host.ansible_facts = {}
            host.ansible_facts_modified = now()
            hosts_to_update.append(host)
            logger.info(f'Facts cleared for inventory {smart_str(host.inventory.name)} host {smart_str(host.name)}')
            log_data['cleared_ct'] += 1

        if len(hosts_to_update) >= 100: