Skip to content

Conversation

@AlexanderSambale
Copy link

@AlexanderSambale AlexanderSambale commented Aug 26, 2025

Purpose

Describe the purpose of this pull request. Why is this change necessary? What problem does it solve?

This pull request is for integrating the folio-custom-fields backend library into mod-inventory-storage, so that it supports storing custiom field values on inventory items.

Approach

How does this change fulfill the purpose? Provide a high-level overview of the technical approach taken to address the problem.

We followed the guide in https://github.com/folio-org/folio-custom-fields for implementing the library and used the implementation in https://github.com/folio-org/mod-orders-storage as reference.

Changes Checklist

  • API Changes: Document any API paths, methods, request or response bodies changed, added, or removed.
  • Database Schema Changes: Indicate any database schema changes and their impact. Confirm that migration scripts were created.
  • Interface Version Changes: Indicate any changes to interface versions.
  • Interface Dependencies: Document added or removed dependencies.
  • Permissions: Document any changes to permissions.
  • Logging: Confirm that logging is appropriately handled.
  • Unit Testing: Confirm that changed classes were covered by unit tests.
  • Integration Testing: Confirm that changed logic was covered by integration tests.
  • Manual Testing: Confirm that changes were tested on local or dev environment.
  • NEWS: Confirm that the NEWS file is updated with relevant information about the changes made in this pull request.

New API

GET, PUT, POST /custom-fields
GET, PUT, DELETE /custom-fields/{id}
GET /custom-fields/{id}/stats
GET /custom-fields/{id}/options/{optId}/stats

Permissions

inventory-storage.custom-fields.all
inventory-storage.custom-fields.collection.get
inventory-storage.custom-fields.collection.put
inventory-storage.custom-fields.item.post
inventory-storage.custom-fields.item.get
inventory-storage.custom-fields.item.put
inventory-storage.custom-fields.item.delete
inventory-storage.custom-fields.item.stats.get
inventory-storage.custom-fields.item.option.stats.get

Related Issues

List any Jira issues related to this pull request.

https://folio-org.atlassian.net/browse/MODINVSTOR-1446
https://folio-org.atlassian.net/browse/UXPROD-5370

Learning and Resources (if applicable)

Discuss any research conducted during the development of this pull request. Include links to relevant blog posts, patterns, libraries, or addons that were used to solve the problem.

https://github.com/folio-org/folio-custom-fields
https://github.com/folio-org/mod-orders-storage
https://www.baeldung.com/java-passing-method-parameter

Screenshots (if applicable)

If this pull request involves any visual changes or new features, consider including screenshots or GIFs to illustrate the changes.

mhamann-ubl and others added 3 commits August 26, 2025 14:50
add missing permissions

add customFields to item.json

add RecordServiceFactory implementation

add RecordService meta info

add db indexes for custom fields
Correct permissionsRequired for custom-fields
Correct custom-fields pathPatterns for stats
Add CustomFieldsApiTest to StorageTestSuite.java
Move custom_fields objects in db_scripts  to correct location
Move folio-custom-fields version to prod-deps group
Set custom-fields interface to 3.0
Add custom_fields.json and use it as ref in item.json
Mock /users for CustomFieldsApiTest because of dependencies in custom fields library
Test /custom-fields POST, GET, DELETE, PUT APIs and check if referenced items are updated where necessary.
@sonarqubecloud
Copy link

@AlexanderSambale AlexanderSambale marked this pull request as ready for review August 26, 2025 14:19
@AlexanderSambale AlexanderSambale requested a review from a team as a code owner August 26, 2025 14:19
Copy link
Collaborator

@psmagin psmagin left a comment

Choose a reason for hiding this comment

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

It was agreed that custom-field work will not be merged into the master until it is tested for performance.

@AlexanderSambale
Copy link
Author

It was agreed that custom-field work will not be merged into the master until it is tested for performance.

https://folio-org.atlassian.net/wiki/spaces/FOLIJET/pages/1104609332/Custom+Fields+Support+for+Inventory+Entities+Items+Holdings+Instances?focusedCommentId=1129054240

@psmagin
Just to make it clear, if I understood you correctly:

  • We should test the performance when you delete or update an instance of a custom field which could potentially update many item records, if they reference the custom field.
  • We will make a new branch from here and add additional performance tests.
  • What metrics would you use to measure the performance? What would be an acceptable performance? How many items would you want to test? Is there something to compare to?

@psmagin
Copy link
Collaborator

psmagin commented Aug 28, 2025

@AlexanderSambale

We should test the performance when you delete or update an instance of a custom field which could potentially update many item records, if they reference the custom field.

Correct

We will make a new branch from here and add additional performance tests.

Correct

What metrics would you use to measure the performance?

Execution time, database load.

What would be an acceptable performance?

This should be defined by the PO, but it must definitely be fast enough not to affect other flows (such as data import, bulk update, etc.).

How many items would you want to test?

I would expect tests to run on a bugfest-like dataset with about 8M items. It’s possible that each item could reference the same custom field.

Is there something to compare to?

Yes. There are several performance tests for data import and bulk update. You could reach out to the PTF team, who usually measure performance, and ask for their assistance with this.

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