Skip to content

Conversation

@Chaffelson
Copy link
Owner

Description

This PR implements consistent entity resolution patterns across nipyapi, addressing #396. Functions that previously required specific object types or only accepted ID strings now accept objects, IDs, or names with automatic type detection.

Motivation

Agents and users frequently trip over inconsistent function signatures:

  • Some functions accept only entity objects (schedule_controller)
  • Some accept ID strings but not names (schedule_process_group)
  • Some have inline resolution patterns that differ in behavior

This PR standardizes all action functions to accept flexible input while maintaining backward compatibility.

Changes

New Utilities (nipyapi/utils.py)

Function Purpose
resolve_entity() Resolve object/ID/name to entity with auto-detection
resolve_schedule_state() Normalize bool/string to scheduling state

Refactored Functions

canvas.py (17 functions):

  • Scheduling: schedule_processor, schedule_port, schedule_controller, schedule_all_controllers, schedule_components, schedule_process_group
  • Controllers: delete_controller, update_controller, verify_controller, get_controller (added greedy)
  • Processors: verify_processor, create_processor
  • Process Groups: delete_process_group, update_process_group, purge_process_group, create_process_group
  • State: get_controller_state, clear_controller_state, get_processor_state, clear_processor_state

versioning.py (9 functions):

  • delete_registry_client, update_registry_client, get_version_info
  • update_git_flow_ver, get_local_modifications, stop_flow_ver, revert_flow_ver
  • save_git_flow_ver, import_process_group_definition

parameters.py (5 functions):

  • delete_parameter_context, delete_parameter_from_context, upsert_parameter_to_context
  • assign_context_to_process_group, remove_context_from_process_group

Additional Changes During Refactoring

The following changes were discovered necessary or beneficial during the refactoring review:

New Functions

  • nipyapi.canvas.schedule_port() - Start/stop/disable ports (gap identified during review)
  • nipyapi.canvas.get_port() - Retrieve port by ID or name (required for schedule_port)

Dead Code Removed

  • get_variable_registry() - Variable Registries removed in NiFi 2.x, function was broken
  • update_variable_registry() - Use Parameter Contexts instead

Consistency Fixes

  • get_controller() - Added missing greedy parameter for consistency with other getters
  • create_process_group() - Made location optional, uses layout.suggest_pg_position()
  • create_funnel() - Made position optional, uses layout.new_flow()

Code Consolidation

  • _run_verification_request() - Extracted common async pattern from verify_controller and verify_processor
  • _check_processor_state() - Consolidated multiple state-checking helpers into parameterized function
  • _check_port_state() - Same consolidation for port scheduling

Configuration

  • PortEntity added to registered_filters in config.py (required for get_port name lookup)

Test Optimization

  • fix_state_flow fixture changed to module scope (saves ~20s per test run)

New Parameters

All refactored functions gain:

  • greedy (bool, default=True) - Partial vs exact name matching
  • identifier_type (str, default="auto") - Force "id" or "name" lookup

Breaking Changes

ValueError instead of AssertionError for invalid scheduling inputs

What changed: Scheduling functions (schedule_controller, schedule_all_controllers, schedule_components) now raise ValueError instead of AssertionError when given invalid scheduled parameter values.

Why: AssertionError is intended for internal invariant violations, not user input validation. ValueError is the standard Python exception for invalid arguments and provides clearer error messages.

Impact: Low. Code catching AssertionError from these functions is rare, and the new ValueError messages are more descriptive. Users who weren't catching exceptions see improved error messages.

Removed Variable Registry functions

What changed: get_variable_registry() and update_variable_registry() removed from nipyapi.canvas.

Why: Variable Registries were deprecated in NiFi 1.10 (2019) and removed entirely in NiFi 2.x. The underlying API endpoints no longer exist, so these functions would fail with cryptic errors if called.

Impact: None for NiFi 2.x users (functions were already broken). Users on older NiFi versions should use Parameter Contexts (nipyapi.parameters) which have been the replacement since NiFi 1.10.

Testing

All tests pass across authentication profiles:

Profile Passed Skipped Time
single-user 660 31 2:17
secure-mtls 660 31 2:56
secure-ldap 667 24 3:32

Examples

# Before: Required entity object
controller = nipyapi.canvas.get_controller("MyController", "name")
nipyapi.canvas.schedule_controller(controller, True)

# After: Accepts name directly
nipyapi.canvas.schedule_controller("MyController", True)

# Auto-detects UUID vs name
nipyapi.canvas.delete_process_group("abc-123-uuid-format")  # ID lookup
nipyapi.canvas.delete_process_group("My Process Group")     # Name lookup

# Explicit control
nipyapi.canvas.schedule_processor("proc", True, identifier_type="name", greedy=False)

Checklist

  • All existing tests pass unchanged (backward compatibility)
  • New tests added for expanded functionality
  • Docstrings updated for all refactored functions
  • Pre-commit checks pass
  • Tested on single-user, secure-mtls, and secure-ldap profiles

- Add get_port(identifier, identifier_type) for retrieving ports by ID or name
- Add schedule_port(port, scheduled) following schedule_processor pattern
- Register PortEntity in config.py for filter_obj support
- Add comprehensive tests for both functions
Previously create_controller() set the name in a separate update call
after creation, but returned the stale object from the initial call.
This caused component.name to show the type name instead of the custom
name.

Now sets name directly in the ControllerServiceDTO during creation,
matching the pattern used by create_processor(). Also defaults to
short type name if no name provided (e.g., 'JsonTreeReader').
Utilities implemented in nipyapi/utils.py:
- resolve_schedule_state(): Standardized bool/string to state normalization
- resolve_entity(): Entity resolution accepting ID, name, or object
  (to be used in next phase)

Scheduling functions consolidated in canvas.py:
- schedule_processor: Uses resolve_schedule_state()
- schedule_port: Uses resolve_schedule_state()
- schedule_controller: Uses resolve_schedule_state(), now accepts string
  states ('ENABLED', 'DISABLED') in addition to bool
- schedule_all_controllers: Same extension
- schedule_components: Now accepts string states ('RUNNING', 'STOPPED')

Breaking change: Invalid scheduled values now raise ValueError instead
of AssertionError. This is a more appropriate exception type.

Tests expanded for new string state acceptance in controller scheduling.
All 660 tests pass.
…oss modules

- Add resolve_entity() and resolve_schedule_state() utilities in utils.py
- Refactor 17 canvas.py functions to accept object, ID, or name
- Refactor 9 versioning.py functions with consistent entity resolution
- Refactor 5 parameters.py functions with consistent entity resolution
- Add schedule_port() and get_port() functions to canvas.py
- Add greedy parameter to get_controller()
- Remove deprecated get_variable_registry/update_variable_registry
- Consolidate verification request pattern with _run_verification_request helper
- Make location optional in create_process_group and create_funnel
- Add PortEntity to registered_filters in config.py
- Optimize fix_state_flow fixture to module scope
- Update tests for new ValueError exceptions and expanded functionality

Closes #396
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR implements consistent entity resolution across nipyapi, addressing issue #396. The main improvement is that functions now accept objects, IDs, or names with automatic type detection, eliminating the inconsistency where some functions only accepted specific object types or ID strings.

Key Changes

  • New utility functions (nipyapi/utils.py): resolve_entity() for automatic entity resolution and resolve_schedule_state() for normalizing bool/string scheduling states
  • Refactored 31 functions across canvas.py, versioning.py, and parameters.py to use consistent entity resolution patterns with new greedy and identifier_type parameters
  • Breaking changes: Scheduling functions now raise ValueError instead of AssertionError for invalid inputs; removed deprecated Variable Registry functions (get_variable_registry, update_variable_registry)

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
nipyapi/utils.py Added resolve_entity() and resolve_schedule_state() utilities with helper functions for consistent entity resolution and state normalization
nipyapi/canvas.py Refactored 17 functions (scheduling, CRUD operations, state management, verification) to accept flexible inputs; added schedule_port() and get_port(); removed deprecated Variable Registry functions; added _run_verification_request() helper
nipyapi/versioning.py Refactored 9 functions (registry client, git flow, version control) to accept flexible inputs with consistent parameter patterns
nipyapi/parameters.py Refactored 5 functions (parameter context management) to accept flexible inputs
nipyapi/config.py Added PortEntity to registered_filters to support name-based port lookups
tests/test_utils.py Added 311 lines of comprehensive unit and integration tests for new utility functions
tests/test_canvas.py Updated tests to verify new flexible input patterns and ValueError exception changes; added tests for schedule_port() and get_port()
tests/test_versioning_registry.py Updated exception assertions from AssertionError to ValueError; added tests for string ID/name inputs
tests/test_parameters.py Added tests for flexible input patterns (ID and name string inputs)
tests/conftest.py Changed fix_state_flow fixture from function to module scope for performance (saves ~20s per test run); removed unused Variable Registry test data

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@codecov
Copy link

codecov bot commented Jan 6, 2026

Codecov Report

❌ Patch coverage is 95.19231% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 76.47%. Comparing base (c4127de) to head (51339ee).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
nipyapi/canvas.py 93.84% 8 Missing ⚠️
nipyapi/utils.py 95.74% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #398      +/-   ##
==========================================
+ Coverage   75.91%   76.47%   +0.56%     
==========================================
  Files          41       41              
  Lines        4738     4732       -6     
==========================================
+ Hits         3597     3619      +22     
+ Misses       1141     1113      -28     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

Copilot AI commented Jan 6, 2026

@Chaffelson I've opened a new pull request, #399, to work on those changes. Once the pull request is ready, I'll request review from you.

Copy link
Collaborator

@ottobackwards ottobackwards left a comment

Choose a reason for hiding this comment

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

All the entity resolutions now possibly raise exceptions, but the exceptions are not in the docstring.

It should be, so they know to handle exceptions.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Chaffelson and others added 2 commits January 6, 2026 18:38
…ation (#399)

* Initial plan

* docs: Clarify test comment about ValueError from resolve_entity validation

Co-authored-by: Chaffelson <[email protected]>

---------

Co-authored-by: copilot-swe-agent[bot] <[email protected]>
Co-authored-by: Chaffelson <[email protected]>
@Chaffelson
Copy link
Owner Author

All the entity resolutions now possibly raise exceptions, but the exceptions are not in the docstring.
It should be, so they know to handle exceptions.

Good spot, should be fixed now

Copy link
Collaborator

@ottobackwards ottobackwards left a comment

Choose a reason for hiding this comment

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

ship it

@Chaffelson Chaffelson merged commit 0716aee into main Jan 7, 2026
12 checks passed
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