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

feat : Add error attribute to Collection Indexes and Attributes #4575

Merged
merged 18 commits into from
Jun 22, 2023

Conversation

everly-gif
Copy link
Contributor

What does this PR do?

Add error attribute to Collection Indexes and Attributes

Test Plan

  • Manual

Related PRs and Issues

NIL

Have you added your change to the Changelog?

yes

Have you read the Contributing Guidelines on issues?

yes

Copy link
Member

@eldadfux eldadfux left a comment

Choose a reason for hiding this comment

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

Great work, left a few comments and points for discussion. Some extra adjustments in utopia/database might be required.

app/config/collections.php Outdated Show resolved Hide resolved
app/workers/databases.php Show resolved Hide resolved
src/Appwrite/Utopia/Response/Model/Attribute.php Outdated Show resolved Hide resolved
src/Appwrite/Utopia/Response/Model/Index.php Outdated Show resolved Hide resolved
'indexes',
$index->getId(),
$index
->setAttribute('status', 'stuck')
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
->setAttribute('status', 'stuck')
->setAttribute('status', 'stuck')

What status types do we support? can we change this to error? cc @christyjacob4

Copy link
Member

Choose a reason for hiding this comment

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

We currently support

  1. available
  2. failed
  3. stuck
  4. deleting
  5. processing

Copy link
Member

@christyjacob4 christyjacob4 Oct 26, 2022

Choose a reason for hiding this comment

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

failed might make the most sense here. Do we want to deprecate the stuck status ?

'attributes',
$attribute->getId(),
$attribute
->setAttribute('status', 'stuck')
Copy link
Member

Choose a reason for hiding this comment

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

We should deprecate stuck and just use failed instead

Copy link
Contributor Author

@everly-gif everly-gif Oct 26, 2022

Choose a reason for hiding this comment

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

considering this comment here, i think changing it to failed can be misleading.

failed indicates the attribute was never created in the db, whereas stuck
indicates the attribute is available in the db but the deletion operation failed. Since the exception thrown here would only be of a case where the attribute is available in the db, I think stuck makes sense by the definition. If we want to deprecate stuck and go with failed i think we should redefine failed and what it means before doing so.

composer.json Outdated Show resolved Hide resolved
@abnegate abnegate changed the base branch from master to 1.4.x June 15, 2023 01:02
@abnegate abnegate mentioned this pull request Jun 15, 2023
2 tasks
@abnegate abnegate merged commit 35d0651 into 1.4.x Jun 22, 2023
1 of 2 checks passed
@abnegate abnegate deleted the add-error-attribute branch October 25, 2023 04:46
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.

None yet

4 participants