Skip to content

Conversation

@PBobylev
Copy link
Contributor

No description provided.

@PBobylev PBobylev self-assigned this Oct 23, 2025
var existed = resourceRepository.existsById(resource.getId());
var entity = resourceModelMapper.toEntity(resource);
var saveGraphResult = resourceGraphService.saveMergingGraph(entity);
if (existed) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if we need to check for existence of a resource.
On one hand, this allows us to choose between CREATE and UPDATE events, but on the other hand, it definitely degrades performance.

Let's discuss it.

Copy link
Contributor

@pkjacob pkjacob Oct 23, 2025

Choose a reason for hiding this comment

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

Actually, we can check in "newResources" array of "saveGraphResult" to determine if entity is already existing or not.

var existed = !saveGraphResult.newResources().contains(entity);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh actually a saveGraphResult already contains distinguished info regarding created and updated resources, right?
So why do we need to manually decide if create or update events should be sent.. I will refactor ResourceEventsPublisher to do it automatically

Copy link
Contributor

@pkjacob pkjacob Oct 24, 2025

Choose a reason for hiding this comment

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

When the work / instance is edited via UI, we delete the original work / instance and create a new one.
So, the new work (which replaced the original work) will be always present in the newResources() array.
So, we manually invoke emitEventsForUpdate from PUT method and emitEventsForCreate from POST method.

Copy link
Contributor Author

@PBobylev PBobylev Oct 24, 2025

Choose a reason for hiding this comment

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

In the case you described we also have or do NOT have the oldResource object and this lets us distinguish the type of the process happening

Copy link
Contributor

Choose a reason for hiding this comment

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

You are right. Good idea.

Copy link
Contributor

@pkjacob pkjacob Oct 24, 2025

Choose a reason for hiding this comment

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

Can we make a small change in saveMergingGraph method? At present, we only search in createdResources array for persistedRootResource. The persistedRootResource could be in updatedResources also.
So, maybe we have to change as follows

    var persistedRootResource = filterResources(result, r -> r.resource().getId().equals(resource.getId()))
      .stream()
      .findFirst()
      .orElse(resource);

@pkjacob pkjacob self-requested a review October 23, 2025 21:07
@sonarqubecloud
Copy link

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