Skip to content
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.

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

MODINV-455: Inventory created more Instances records than were in the file #375

Merged
merged 1 commit into from Jul 12, 2021

Conversation

Aliaksandr-Fedasiuk
Copy link
Contributor

@Aliaksandr-Fedasiuk Aliaksandr-Fedasiuk commented Jul 9, 2021

Purpose

Inventory created more Instances records than were in the file because it generates a unique id for each request

Approach

In our distributed system, we can use the unique id already created by another module, since these entities are in the 1-2-1 relationship

Learning

https://issues.folio.org/browse/MODINV-455

@sonarcloud
Copy link

sonarcloud bot commented Jul 9, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@marcjohnson-kint
Copy link
Member

In our distributed system, we can use the unique id already created by another module, since these entities are in the 1-2-1 relationship

Does this mean using the same UUID for the id of two different records (of different record types)?

@Aliaksandr-Fedasiuk
Copy link
Contributor Author

In our distributed system, we can use the unique id already created by another module, since these entities are in the 1-2-1 relationship

Does this mean using the same UUID for the id of two different records (of different record types)?

yes. these records will not overlap and are located in different schemes, but this decision will accurately block the system from duplicates without additional costs. We can, of course, to add a separate unique field, but this will lead to a new additional complication of the system (writing of new migration scripts, fields in objects, and etc.)

@marcjohnson-kint
Copy link
Member

marcjohnson-kint commented Jul 9, 2021

yes. these records will not overlap and are located in different schemes, but this decision will accurately block the system from duplicates without additional costs.

My understanding is that FOLIO chose UUID for it's ID properties so that they would be globally unique and it wasn't intended that the same ID be used for two different records (even if they are different types)

We can, of course, to add a separate unique field, but this will lead to a new additional complication of the system (writing of new migration scripts, fields in objects, and etc.)

I don't know what the specific use case is. If one of the record types relates to the other, I might want that relationship to be made explicit by having a property named for that purpose.

I don't have any authority over this decision, it is for you and your team to make. I only wanted to understand better and share my understanding of prior decisions.

Given this change has been approved already, that suggests folks are happy with it. I won't comment further.

@Aliaksandr-Fedasiuk
Copy link
Contributor Author

yes. these records will not overlap and are located in different schemes, but this decision will accurately block the system from duplicates without additional costs.

My understanding is that FOLIO chose UUID for it's ID properties so that they would be globally unique and it wasn't intended that the same ID be used for two different records (even if they are different types)

We can, of course, to add a separate unique field, but this will lead to a new additional complication of the system (writing of new migration scripts, fields in objects, and etc.)

I don't know what the specific use case is. If one of the record types relates to the other, I might want that relationship to be made explicit by having a property named for that purpose.

I don't have any authority over this decision, it is for you and your team to make. I only wanted to understand better and share my understanding of prior decisions.

Given this change has been approved already, that suggests folks are happy with it. I won't comment further.

My team agrees with this, but it's good that you point it out. Thank you

@Aliaksandr-Fedasiuk Aliaksandr-Fedasiuk merged commit 634bfd5 into master Jul 12, 2021
@Aliaksandr-Fedasiuk Aliaksandr-Fedasiuk deleted the MODINV-455 branch July 12, 2021 14:07
KaterynaSenchenko pushed a commit that referenced this pull request Jul 15, 2021
KaterynaSenchenko pushed a commit that referenced this pull request Jul 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants