Fixed to retain static items when reloading(#57404)

[ivis-miyachi]

No description provided.

[Copilot]

Pull request overview

This pull request fixes issue #57404 by implementing functionality to retain static items (values starting with '=') when reloading item type mappings. The fix ensures that configuration values marked as static are preserved during the reload operation rather than being overwritten.

Changes:

• Added retrieval of existing item type mapping at the start of the reload process
• Introduced logic to merge static values from old mapping into new mapping before updating
• Implemented a new utility method update_mapping_without_static to handle the static value preservation logic

Reviewed changes

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

File	Description
modules/weko-records/weko_records/api.py	Added item_type_mapping retrieval and integration with new update_mapping_without_static method in the reload method; implemented new update_mapping_without_static class method with nested helper function to recursively find and preserve static values
modules/weko-records/tests/test_api.py	Added unit test test_update_mapping_without_static to verify the static value preservation logic with various scenarios including nested dictionaries and non-existent paths

---

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

[Copilot]

The code doesn't check if temp[key] is a dictionary before attempting to access it in the next iteration. If an intermediate value in the path is not a dictionary (e.g., it's a string or list), this will raise a TypeError when trying to use it as a dictionary in the next iteration. Consider adding a type check: if key not in temp or not isinstance(temp[key], dict):

Suggested change

	if key not in temp:
	is_exist_field = False
	break
	temp = temp[key]
	if is_exist_field:
	# Ensure the current container is a dict and contains the key,
	# and that the next value is also a dict (since it will be traversed).
	if not isinstance(temp, dict) or key not in temp or not isinstance(temp[key], dict):
	is_exist_field = False
	break
	temp = temp[key]
	if is_exist_field and isinstance(temp, dict):

[Copilot]

Missing space around comparison operators. For consistency with PEP 8 and the rest of the codebase, add spaces: if path != "" else key

Suggested change

	result+=find_static_value(value,f"{path}.{key}" if path!="" else key)
	result+=find_static_value(value,f"{path}.{key}" if path != "" else key)

[Copilot]

Missing space around augmented assignment operator. For consistency with PEP 8, add spaces: result += find_static_value(...)

Suggested change

	if isinstance(d,dict):
	for key, value in d.items():
	result+=find_static_value(value,f"{path}.{key}" if path!="" else key)
	elif isinstance(d,str):
	if d.startswith("="):
	result.append((path,d))
	if isinstance(d, dict):
	for key, value in d.items():
	result += find_static_value(value, f"{path}.{key}" if path != "" else key)
	elif isinstance(d, str):
	if d.startswith("="):
	result.append((path, d))

[Copilot]

Trailing whitespace after result = []. Remove the trailing space for cleaner code.

Suggested change

	result = []
	result = []

[Copilot]

The test doesn't verify the edge case where an intermediate path exists but is not a dictionary (e.g., {'a': 'string_value'} when trying to access a.b). This could lead to a TypeError at runtime. Consider adding a test case to ensure the method handles this scenario gracefully.

[Copilot]

The test doesn't verify that the method handles empty dictionaries correctly. Consider adding test cases for edge cases like old_data = {} and new_data = {}.

[Copilot]

The line continuation with backslash could be improved for readability. Consider wrapping the conditional expression in parentheses instead of using a backslash, which is the preferred style in Python. This would make the code more maintainable and avoid potential issues with trailing whitespace after the backslash.

Suggested change

	update_mapping = cls.update_mapping_without_static(item_type_mapping.get(_prop_id,{}),mapping_dict.get(_property_id)) \
	if item_type_mapping else mapping_dict.get(_property_id)
	update_mapping = (
	cls.update_mapping_without_static(
	item_type_mapping.get(_prop_id, {}),
	mapping_dict.get(_property_id),
	)
	if item_type_mapping
	else mapping_dict.get(_property_id)
	)

[Copilot]

The test verifies that static values with non-existent paths are ignored (see "not_exist_path" in old_data), which correctly validates the documented behavior. However, consider adding a test case to verify deeper nesting levels (3+ levels deep) to ensure the path traversal logic works correctly for more complex structures.

[Copilot]

Missing space around comparison operators in tuple unpacking. For consistency with PEP 8, format as: result.append((path, d))

Suggested change

	result.append((path,d))
	result.append((path, d))

[Copilot]

According to the docstring, "Only existing paths in mapping_prop will be updated; missing paths are ignored." However, the code doesn't check if the final key exists before updating it. If keys[-1] doesn't exist in temp, the code will still create it. Consider adding: if is_exist_field and keys[-1] in temp: to align with the documented behavior.

Suggested change

	if is_exist_field:
	if is_exist_field and keys[-1] in temp: