Skip to content

Conversation

@may-hartov
Copy link
Collaborator

@may-hartov may-hartov commented Jan 1, 2026

Summary

Public Fabric APIs for updating resources (capacities, workspaces, items, etc.) are exposed as REST PATCH operations and expect request bodies to include only the fields being changed.
Our CLI set command previously fetched the full resource payload via GET, modified a single field (for example, displayName), and sent the entire object back to the update endpoint. This behavior does not align with PATCH semantics and can result in unexpected behavior or validation errors.
This PR refactors the update flow to construct PATCH request bodies correctly by extracting only the updated properties from the GET response.

Changes

  • fab_cmd_set_utils:

    1. Extracted update_item_definition from update_fabric_element, as it is item-specific.
    2. Removed json_payload from update_fabric_element return values (unused by callers).
    3. Added extract_updated_only: bool = True to update_fabric_element to control whether only the updated path is extracted or the full payload is returned. For OneLake shortcut updates, this flag is set to False, since shortcuts use the Create API rather than an Update API.
    4. Introduced update_cache to reduce code duplication across fab_fs_set_* implementations.
  • fab_fs_set_*
    Removed data.pop(...) calls. Since the payload now includes only the target path being updated, fields like id are no longer present and do not need to be stripped.

  • fab_fs_set_connection
    Added connectivityType to the payload, as required by the Update Gateway API documentation.

  • fab_fs_set_gateway
    Added the required type field, per the [Update Gateway API documentation.]((https://learn.microsoft.com/en-us/rest/api/fabric/core/connections/update-gateway).

Tests
Because this change affects how request payloads are constructed for update APIs, all set command tests were re-recorded to reflect the new behavior.

@may-hartov may-hartov requested a review from a team as a code owner January 1, 2026 12:24
@may-hartov may-hartov changed the title refactor(set): refactors the update flow to construct PATCH request bodies correctly by extracting only the updated properties from the GET response. refactor(set): refactor the update flow to construct PATCH request bodies correctly Jan 1, 2026
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 refactors the set command implementation to properly align with REST PATCH semantics by sending only the updated properties rather than the entire resource payload. Previously, the CLI would fetch the full resource via GET, modify a single field, and send the entire object back via PATCH, which doesn't align with PATCH semantics and could cause validation errors.

Key changes:

  • Extracted update_item_definition from update_fabric_element for item-specific base64 handling
  • Added extract_updated_only parameter to control payload extraction behavior
  • Introduced update_cache helper to reduce code duplication
  • Updated tests to reflect new payload structure and cache behavior

Reviewed changes

Copilot reviewed 41 out of 45 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/fabric_cli/utils/fab_cmd_set_utils.py Core refactoring: split item definition handling, added extract_updated_only flag, new update_cache helper
src/fabric_cli/errors/common.py Added gateway-specific error messages for unsupported operations
tests/test_utils/test_fab_cmd_set_utils.py Added test for update_item_definition, updated assertions for extracted properties
tests/test_commands/test_set.py Updated cache assertion logic to only expect cache updates for displayName or name changes
tests/test_commands/recordings/**/*.yaml Re-recorded VCR tests showing minimal PATCH payloads instead of full resource objects
tests/test_commands/api_processors/capacities_api_processor.py Added defensive checks for optional fields in request/response handling
.changes/unreleased/optimization-20260101-115402.yaml Changelog entry for the optimization

"""
try:
input = json.loads(input)
except (TypeError, json.JSONDecodeError):
Copy link

Copilot AI Jan 1, 2026

Choose a reason for hiding this comment

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

'except' clause does nothing but pass and there is no explanatory comment.

Copilot uses AI. Check for mistakes.
query_value,
input_value,
)
except (ValueError, KeyError, IndexError):
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe use something more generic, like except Exception:


utils_ui.print_grey(f"Setting new property for '{virtual_ws_item.name}'...")
response = capacity_api.update_capacity(args, capacity_update_def)
response = capacity_api.update_capacity(args, json.dumps(updated_def, indent=4))
Copy link
Collaborator

Choose a reason for hiding this comment

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

why indent=4 necessary?

input: str,
decode_encode: bool = False,
) -> tuple[str, dict]:
extract_updated_only: bool = True,
Copy link
Collaborator

Choose a reason for hiding this comment

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

the name is a bit amigious and unclear. maybe something like 'return_modified_properties' or something like that

}

definition_properties = None
def update_cache(
Copy link
Collaborator

Choose a reason for hiding this comment

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

to be generic and used in other places (like sparkpool), consider getting another argument of property name and use it.

something like

def update_catch(
    updated_def: dict,
    element,
    cache_update_func,
    element_name_key = "displayName"
) -> None:
    """Update element's display name in cache if it was changed.

    Args:
        updated_def: Dictionary containing the updated properties.
        element: The Fabric element whose name should be updated.
        cache_update_func: Function to call to update the cache.
        element_name_key: Key of the element name property. Default to 'displayName'.

    """
    if element_name_key in updated_def:
        element._name = updated_def[element_name_key]
        cache_update_func(element)

workspace._name = name_description_properties["displayName"]
utils_mem_store.upsert_workspace_to_cache(workspace)

utils_set.update_cache(
Copy link
Collaborator

Choose a reason for hiding this comment

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

really liked this refactor!

Comment on lines +132 to +135
decoded_item_def = _decode_payload(item_def)
decoded_item_def = ensure_notebook_dependency(decoded_item_def, query)
updated_item_def = utils_jmespath.replace(decoded_item_def, query, input)
updated_def = _encode_payload(updated_item_def)
Copy link
Collaborator

Choose a reason for hiding this comment

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

instead of having this new method, can we reuse the update_fabric_element with another argument like 'update_definition', when true it will do this decode-replace-encode. does that make sense?

Comment on lines +48 to +56
elif gatewat_type == "OnPremises" and query.startswith(
("numberOfMemberGateways", "capacityId", "inactivityMinutesBeforeSleep")
):
raise FabricCLIError(
ErrorMessages.Common.gateway_property_not_supported_for_type(
query, "OnPremises"
),
fab_constant.ERROR_NOT_SUPPORTED,
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

is it for early failure detection instead of getting api failure?

@may-hartov may-hartov closed this Jan 1, 2026
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