Skip to content

Feat: Implement updateAttribute()#124

Closed
Meldiron wants to merge 9 commits intomainfrom
feat-methods-update-attribute
Closed

Feat: Implement updateAttribute()#124
Meldiron wants to merge 9 commits intomainfrom
feat-methods-update-attribute

Conversation

@Meldiron
Copy link
Contributor

@Meldiron Meldiron commented Apr 7, 2022

No description provided.

@Meldiron
Copy link
Contributor Author

Meldiron commented Apr 7, 2022

Mongo tests are expected to fail, implementation missing.

@Meldiron
Copy link
Contributor Author

Letting the database natively rebuild index (and decide when it needs to do it) is a better solution because:

  • If it fails, the whole thing fails. You don't need to rollback anything
  • The index regeneration SHOULD behave the same as CREATE INDEX, so there isn't any advantage or disadvantage

@TorstenDittmann
Copy link
Contributor

Looking at this, it might have been smarter to implement more smaller functions instead of a universal one 🤔

@Meldiron
Copy link
Contributor Author

Looking at this, it might have been smarter to implement more smaller functions instead of a universal one 🤔

Yes, there is also an edge case when index rebuild might be triggered, and query results would depend on how strict MariaDB settings are. Before making this user-faced in Appwrite, we need bigger research (what queries cost more) and separate index-rebuilding-logic from the one that doesn't. For now, for migration purposes, this should be quick and simple solution.

Do you want me to work on improving this PR?

->setAttribute('signed', $signed)
->setAttribute('array', $array);

$success = $this->adapter->updateAttribute($collection->getId(), $id, $type, $size, $signed, $array);
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't we want to throw an exception here if something fails?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will throw automatically, providing a proper message of what happened. For instance Truncated incorrect INTEGER value: "Matej"

Copy link
Contributor

Choose a reason for hiding this comment

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

So, why do we need to keep success?

Let's say it returns false -> indicating that something went wrong. We are still executing all of its remaining code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I see the confusion now. First, I added return $success as it was missing.

The boolean comes from PDO->execute(). Not sure why it returns a boolean but also throws an exception. Looking at other adapter methods, they all return execute(), so they return a boolean.

Copy link
Contributor

Choose a reason for hiding this comment

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

Now we are returning it too early 👍🏻

The PDO has a configuration, where we can set either throw an exception or return false/true. Let's check if its false here and return false. If not just continue. At the bottom we can just return true. That way we make sure, we do actions only when previous steps are successful.

@Meldiron
Copy link
Contributor Author

Closing PR. Problem addressed in separate PR that makes more developer-friendly interfaces: #132

Most of the code copied from this PR.

@Meldiron Meldiron closed this May 15, 2022
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