Skip to content

fixed: delete attribute even if related collection does not exist - #6045

Closed
Akshay-Rana-Gujjar wants to merge 13 commits into
appwrite:mainfrom
Akshay-Rana-Gujjar:fix-6012-limbo-attribute-if-related-collection-is-deleted
Closed

Akshay-Rana-Gujjar wants to merge 13 commits into
appwrite:mainfrom
Akshay-Rana-Gujjar:fix-6012-limbo-attribute-if-related-collection-is-deleted

Conversation

@Akshay-Rana-Gujjar

Copy link
Copy Markdown
Contributor

What does this PR do?

Delete attribute even if related collection does not exist.

Test Plan

(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work. Screenshots may also be helpful.)

Related PRs and Issues

Checklist

  • Have you read the Contributing Guidelines on issues?
  • If the PR includes a change to an API's metadata (desc, label, params, etc.), does it also include updated API specs and example docs?

@fanatic75
fanatic75 self-requested a review September 1, 2023 18:38

@fanatic75 fanatic75 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@Akshay-Rana-Gujjar , Let's add a test case for this scenario

@Akshay-Rana-Gujjar

Copy link
Copy Markdown
Contributor Author

Hey @fanatic75, I have added the test case, please review 🙏

@Akshay-Rana-Gujjar

Copy link
Copy Markdown
Contributor Author

Hey @fanatic75 , any update on this?

@fanatic75
fanatic75 self-requested a review October 5, 2023 05:41
@Akshay-Rana-Gujjar

Copy link
Copy Markdown
Contributor Author

@fanatic75 please review 🙏

@Akshay-Rana-Gujjar

Copy link
Copy Markdown
Contributor Author

Hey @stnguyen90, Could you please help to review this PR.

Comment thread tests/e2e/Services/Databases/DatabasesBase.php Outdated
Comment thread tests/e2e/Services/Databases/DatabasesBase.php Outdated
Comment thread tests/e2e/Services/Databases/DatabasesBase.php
Comment thread tests/e2e/Services/Databases/DatabasesBase.php

@fanatic75 fanatic75 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just fix these small changes
Otherwise looks good to me. Good work!

Comment thread tests/e2e/Services/Databases/DatabasesBase.php Outdated
Comment thread tests/e2e/Services/Databases/DatabasesBase.php Outdated
Comment thread tests/e2e/Services/Databases/DatabasesBase.php Outdated

@fanatic75 fanatic75 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, @abnegate please review

@Akshay-Rana-Gujjar

Copy link
Copy Markdown
Contributor Author

@abnegate please help review this PR, thanks

@Akshay-Rana-Gujjar

Copy link
Copy Markdown
Contributor Author

@stnguyen90 Can you please help with this merge it is almost 1 month and It is open 😢

@Akshay-Rana-Gujjar

Copy link
Copy Markdown
Contributor Author

Hey @Haimantika & @tessamero, could you please help merge this MR? It has been open for quite some time.

@abnegate abnegate left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

How do you get to the state where the related collection doesn't exist but the relationship does? Are there steps to reproduce?

This sounds like the bug might be related to deleting a collection itself (which should delete all of it's relationships)

@Akshay-Rana-Gujjar

Copy link
Copy Markdown
Contributor Author

How do you get to the state where the related collection doesn't exist but the relationship does? Are there steps to reproduce?

This sounds like the bug might be related to deleting a collection itself (which should delete all of it's relationships)

Yes, I followed the steps mentioned in this issue.

👟 Reproduction steps

  1. Create collection level1
  2. Create collection level2
  3. Create attribute in level1
    • type: relationship
    • one/two way: one way
    • key: level2
    • related collection: level2
    • relation: Many to One
    • on deleting: Cascade
  4. Delete collection level2
  5. Delete attribute level2 from collection level1

@fogelito

Copy link
Copy Markdown
Contributor

I agree, we should not get to this point we have a hanging attribute, but at least we should be able to fix it if it happens

@Akshay-Rana-Gujjar

Copy link
Copy Markdown
Contributor Author

I agree, we should not get to this point we have a hanging attribute, but at least we should be able to fix it if it happens

Hey @fogelito, could you please explain what I need to do further to get approvals and merge this MR? Thanks.

@fogelito

Copy link
Copy Markdown
Contributor

Hey, thanks a lot 🙏 , @abnegate can you give your review here, please?

@abnegate

Copy link
Copy Markdown
Member

@Akshay-Rana-Gujjar The root cause of the issue is that when a collection is deleted, the relationships are not deleted from the attributes table for the related collection, but are deleted from the metadata. So when trying to delete it manually later, it can't be found (in metadata) and gets stuck. Can we pivot the fix to address that instead?

@stnguyen90

Copy link
Copy Markdown
Contributor

Closing as we've updated the logic to delete the related attributes when a collection is deleted.

@stnguyen90 stnguyen90 closed this Apr 26, 2024
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.

5 participants