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

MODLD-593 (Spike): Retaining admin metadata after edit #56

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

pkjacob
Copy link
Contributor

@pkjacob pkjacob commented Nov 21, 2024

No description provided.

@pkjacob pkjacob marked this pull request as draft November 21, 2024 22:22
@pkjacob pkjacob changed the title MODLD-593 (Spike): Retaining admin metadata MODLD-593 (Spike): Retaining admin metadata after edit Nov 21, 2024
@pkjacob pkjacob force-pushed the pjacob/MODLD-593_2 branch 3 times, most recently from 44ace93 to a59c8a0 Compare November 22, 2024 01:31
}

public void copyOutgoingEdges(Resource from, Resource to) {
this.rules.entrySet()
Copy link
Contributor

@PBobylev PBobylev Nov 22, 2024

Choose a reason for hiding this comment

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

Idea:
What if instead of hard-coded list of resource types/predicates we do it dynamically?
We already have a way to find a mapper by resource type / predicate / request/response dto:
org.folio.linked.data.mapper.dto.common.SingleResourceMapperImpl#getMapperUnit
So, might be we will copy every resource which doesn't have a mapper?

Copy link
Contributor Author

@pkjacob pkjacob Nov 26, 2024

Choose a reason for hiding this comment

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

Hi @PBobylev, There is one problem with this approach.

Let us assume the current graph is like this
image

If a cataloger open this work in editor, remove the connection to Person 1, change the creator & illustrator to Person2, and then save, the resulting graph will look like below.
image

Problem - We will keep the "illustrator" connection as org.folio.linked.data.mapper.dto.common.SingleResourceMapperImpl#getMapperUnit will return an empty optional for the http://bibfra.me/vocab/relation/illustrator predicate.

One (very bad) option is to add "isExcludedEdge" method as I did in this PR. I do not like it at all. Also, I think it is dangerous. We will have similar situations in future (eg: after implementing MODLD-361). If the developer forgets to update "isExcludedEdge" method with additional edges, the graph will get corrupted.

So, I prefer going back to the original approach where we control what edges has to be copied over. This way, we have better control on what edges to retain.

Thoughts?


from.getOutgoingEdges()
.stream()
.filter(edge -> !isExcludedEdge(edge))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do not like this. Just added it here as a DRAFT.

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.

2 participants