Skip to content

Fixed to retain static items when reloading(#57404) #1812

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ivis-miyachi merged 1 commit into from
Jan 29, 2026
Merged

Fixed to retain static items when reloading(#57404) #1812

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 25 additions & 1 deletion modules/weko-records/tests/test_api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1002,7 +1002,31 @@ def test_update_attribute_options(self, app):
expected_dict = {"isHide": False, "isShowList": False, "isNonDisplay": False, "isSpecifyNewline": False, "required": False,"key": "key", "type": "fieldset", "items": [{"key": "key.subitem_select_item", "type": "select", "title": "値", "titleMap": [{"name": "a", "value": "a"}, {"name": "b", "value": "b"}], "title_i18n": {"en": "Test Value", "ja": "テスト値"}, "title_i18n_temp": {"en": "Test Value", "ja": "テスト値"},'isHide': False,'isNonDisplay': False,'isShowList': False, 'isSpecifyNewline': False,'required': False}], "title": "abcdef", "title_i18n": {"en": "", "ja": ""}}
ItemTypes.update_attribute_options(old_value,new_value,"ALL")
TestCase().assertDictEqual(new_value, expected_dict)

def test_update_mapping_without_static(self):
old_data={
"str_not_static":"b",
"str_static":"=str_static_value",
"dict_not_static":{"a":"b"},
"dict_static":{"not_static":"a","static":"=dict_static_value"},
"not_exist_path":{"not_static":"a","static":"=not_exist_static_value"},
}

new_data={
"str_not_static":"b",
"str_static":"c",
"dict_not_static":{"a":"b"},
"dict_static":{"not_static":"a","static":"b"},
}

test = {
"str_not_static":"b",
"str_static":"=str_static_value",
"dict_not_static":{"a":"b"},
"dict_static":{"not_static":"a","static":"=dict_static_value"}
}

result = ItemTypes.update_mapping_without_static(old_data,new_data)
assert result==test
Comment on lines +1005 to +1029
Copy link

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

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

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 uses AI. Check for mistakes.
Comment on lines +1005 to +1029
Copy link

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

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

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 uses AI. Check for mistakes.
Comment on lines +1005 to +1029
Copy link

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

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

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 uses AI. Check for mistakes.

# class ItemTypeEditHistory(RecordBase):
# .tox/c1/bin/pytest --cov=weko_records tests/test_api.py::test_item_type_edit_history -v -s -vv --cov-branch --cov-report=term --cov-config=tox.ini --basetemp=/code/modules/weko-records/.tox/c1/tmp
Expand Down
70 changes: 68 additions & 2 deletions modules/weko-records/weko_records/api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1013,7 +1013,7 @@ def reload(cls, itemtype_id, mapping_dict, specified_list=[], renew_value='None'
return result

data = pickle.loads(pickle.dumps(item_type.render, -1))

item_type_mapping = Mapping.get_record(itemtype_id)
pat1 = re.compile(r'cus_(\d+)')
for idx, i in enumerate(data['table_row_map']['form']):
if isinstance(i,dict) and 'key' in i:
Expand All @@ -1029,7 +1029,9 @@ def reload(cls, itemtype_id, mapping_dict, specified_list=[], renew_value='None'
if _prop:
# fix mapping
if _property_id in mapping_dict:
data['table_row_map']['mapping'][_prop_id] = 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)
Comment on lines +1032 to +1033
Copy link

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

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

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 uses AI. Check for mistakes.
data['table_row_map']['mapping'][_prop_id] = update_mapping
# data['meta_list'][_prop_id] = json.loads('{"input_maxItems": "9999","input_minItems": "1","input_type": "cus_'+str(_prop.id)+'","input_value": "","option": {"crtf": false,"hidden": false,"multiple": true,"oneline": false,"required": false,"showlist": false},"title": "'+_prop.name+'","title_i18n": {"en": "", "ja": "'+_prop.name+'"}}')
# data['schemaeditor']['schema'][_prop_id]=pickle.loads(pickle.dumps(_prop.schema, -1))
if multiple_flg:
Expand Down Expand Up @@ -1120,6 +1122,70 @@ def reload(cls, itemtype_id, mapping_dict, specified_list=[], renew_value='None'

return result

@classmethod
def update_mapping_without_static(cls, old_mapping, mapping_prop):
"""Update the mapping dictionary by copying static values from the old mapping.

This method searches for static values (strings starting with '=') in the old_mapping,
and sets the same static values at the corresponding paths in mapping_prop.
Only existing paths in mapping_prop will be updated; missing paths are ignored.

Args:
old_mapping (dict): The original mapping dictionary containing static values.
mapping_prop (dict): The target mapping dictionary to update.

Returns:
dict: The updated mapping_prop with static values set.

Example:
>>> old = {'a': {'b': '=static'}}
>>> new = {'a': {'b': 'test'}}
>>> update_mapping_without_static(old, new)
{'a': {'b': '=static'}}
"""

def find_static_value(d, path=""):
"""Recursively search for static values in a nested dictionary structure.

A static value is defined as a string that starts with '='.
Returns a list of tuples, where each tuple contains the path
(dot-separated) to the static value and the value itself.

Args:
d (dict or str): The dictionary or string to search.
path (str, optional): The current path in dot notation. Defaults to "".

Returns:
list of tuple: List of (path, value) pairs for all static values found.

Example:
>>> data = {'a': {'b': '=static'}, 'c': 'value'}
>>> find_static_value(data)
[('a.b', '=static')]
"""
result = []
Copy link

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

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

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

Suggested change
result = []
result = []

Copilot uses AI. Check for mistakes.
if isinstance(d,dict):
for key, value in d.items():
result+=find_static_value(value,f"{path}.{key}" if path!="" else key)
Copy link

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

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

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 uses AI. Check for mistakes.
elif isinstance(d,str):
if d.startswith("="):
result.append((path,d))
Comment on lines +1167 to +1172
Copy link

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

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

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 uses AI. Check for mistakes.
Copy link

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

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

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 uses AI. Check for mistakes.
return result

set_static = find_static_value(old_mapping)
for path, value in set_static:
keys = path.lstrip('.').split('.')
temp = mapping_prop
is_exist_field = True
for key in keys[:-1]:
if key not in temp:
is_exist_field = False
break
temp = temp[key]
if is_exist_field:
Comment on lines +1181 to +1185
Copy link

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

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

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 uses AI. Check for mistakes.
Copy link

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

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

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:

Copilot uses AI. Check for mistakes.
temp[keys[-1]] = value
return mapping_prop

@classmethod
def update_property_enum(cls, old_value, new_value, renew_value = 'None'):
managed_key_list = current_app.config.get("WEKO_RECORDS_MANAGED_KEYS")
Expand Down
Loading