Skip to content

Conversation

@hannyle
Copy link
Contributor

@hannyle hannyle commented Nov 14, 2022

Description

Deleting objects inside a folder should now update the folder's items count and size correctly.

Related issues

Fixes #743

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Changes Made

Testing

  • Tests do not apply

@hannyle hannyle changed the title Fix for updating objects count when deleting objects inside folder Fix for updating objects count and size when deleting objects inside folder Nov 14, 2022
@hannyle hannyle self-assigned this Nov 14, 2022
@hannyle hannyle added the bug Something isn't working label Nov 14, 2022
@hannyle hannyle linked an issue Nov 14, 2022 that may be closed by this pull request
obj.containerID = container.id;
obj.tokens = isSegmentsContainer ? [] : tokenize(obj.name);
});
await state.db.objects.bulkPut(objects).catch(() => {});
Copy link
Contributor

Choose a reason for hiding this comment

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

It's nice to get rid of this :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@csc-felipe
Copy link
Contributor

Could you explain what was wrong with the previous logic, and how you solved it?

@rquazi
Copy link
Member

rquazi commented Nov 15, 2022

I was using some bootstrap nav item class which was not responsive on screen resizing. I need to add some other nav class element to make it responsive. Things are not fixed yet :) I am working on it.

@csc-felipe
Copy link
Contributor

I was using some bootstrap nav item class which was not responsive on screen resizing. I need to add some other nav class element to make it responsive. Things are not fixed yet :) I am working on it.

Did you mean to comment on #790?

@sampsapenna
Copy link
Member

Could you explain what was wrong with the previous logic, and how you solved it?

@csc-felipe bulkPut never replaces the old container or object entries, due to the DB having unique constraints on for [projectID+name]. That's how it's supposed to be done, we don't want multiple same entries with the same names, but because of a .catch(() => {}) added to the end of the bulkPut the failure to add containers that already exist wasn't reported in console. If you remove the catch from it you can see how it actually fails for every entry that already had some value in the database.

Because of this existing containers weren't put into the database or updated in any way. Instead, we needed to check if the container already exists in database, and perform an update on the existing container using it's internal key. Replacing existing entries in the database would've worked if the [projectID+name] actually was the primary key of the entry, but inbound incremental key was used instead. The [projectID+name] index worked to ensure uniqueness though.

The new version actually checks if the old entries are in the database, and just updates them if need be. After giving this a bit more thought removing the ++id from container and object entries might fix this as well, since it should make the DB use [projectID+name] or [containerID+name] as primary key.

@csc-felipe
Copy link
Contributor

Thanks for explaining! Makes me wonder why did I make the incremental ID as primary key. I might have not realized that we could use [projectID+name] or [containerID+name].

Indeed, I'm relieved that .catch(() => {}) is gone. I felt bad about that, and knew it would cause trouble, but then moved to working in a different project, and couldn't continue.

@hannyle
Copy link
Contributor Author

hannyle commented Nov 15, 2022

Thanks @sampsapenna for helping explain this very comprehensively! 💯

@hannyle hannyle closed this Nov 15, 2022
@hannyle hannyle reopened this Nov 15, 2022
@hannyle hannyle merged commit 486c63e into devel Nov 15, 2022
@hannyle hannyle deleted the bugfix/delete-objects branch November 15, 2022 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Deleting objects doesn't update folder items count

5 participants