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

long size varchar with long index size mysql #210

Closed
wants to merge 21 commits into from

Conversation

fogelito
Copy link
Contributor

No description provided.

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.

There are a lot of leftovers, not sure if the PR is ready or not, but left some comments.

@@ -1184,7 +1185,10 @@ public function createIndex(string $collection, string $id, string $type, array
}

$collection = $this->silent(fn() => $this->getCollection($collection));

if($collection->isEmpty()){
throw new Exception('Not found');
Copy link
Member

Choose a reason for hiding this comment

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

Are we missing this check from other methods? We should make sure we keep a consistent behavior across the library. Please add this check in other places when absolutely needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok. I added some more checks like this where the collection was called.

@@ -1184,7 +1185,10 @@ public function createIndex(string $collection, string $id, string $type, array
}

$collection = $this->silent(fn() => $this->getCollection($collection));

if($collection->isEmpty()){
throw new Exception('Not found');
Copy link
Member

Choose a reason for hiding this comment

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

Not found is very generic and not helpful at all for other devs who might be using the library. Our errors should be as verbose, useful, and consistent as possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to
throw new Exception('Collection ' .$collectionName . ' Not found');
I added In a few more places we are called the collection object.

@@ -383,6 +396,18 @@ public function createIndex(string $collection, string $id, string $type, array
}, $attributes);

foreach ($attributes as $key => $attribute) {

// $size = (int)($lengths[$key] ?? 0);
Copy link
Member

Choose a reason for hiding this comment

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

Don't we need this part?
Should we throw an exception when index size is not set properly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the Index size, not the attribute size, it is not mandatory.
I commend out this part since I have questions. This is another place we have to fix regardless of the current bug.

@@ -148,34 +148,48 @@ public function createCollection(string $name, array $attributes = [], array $in
$database = $this->getDefaultDatabase();
$namespace = $this->getNamespace();
$id = $this->filter($name);
$collectionAttributes = [];
Copy link
Member

Choose a reason for hiding this comment

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

Not sure what this variable name should indicate. The story of this new logic is not very clear.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed it for functions I think it is much clear now.

src/Database/Adapter.php Outdated Show resolved Hide resolved
@@ -1578,12 +1612,11 @@ public function count(string $collection, array $queries = [], int $max = 0): in
* @return int|float
* @throws Exception
*/
public function sum(string $collection, string $attribute, array $queries = [], int $max = 0)
public function sum(string $collectionName, string $attribute, array $queries = [], int $max = 0)
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
public function sum(string $collectionName, string $attribute, array $queries = [], int $max = 0)
public function sum(string $collection, string $attribute, array $queries = [], int $max = 0)

Let's prioritize naming in the signatures, to keep the external API of the library consistent.

Copy link
Member

Choose a reason for hiding this comment

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

You can overwrite $collection internally and then use $collection->getId()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the collection is empty will it return the name? I need to check

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if $collection->isEmpty()
$collection->getId() is empty as well
throw new Exception('Collection ' .$collection->getId() . ' Not found');


foreach ($attributes as $key => $attribute) {
foreach ($attributes as $attribute) {
Copy link
Member

Choose a reason for hiding this comment

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

We have to explain this logic, not clear what is being achieved here. Please add a docblock.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

* @return Document|null
* @throws Exception
*/
public function findAttributeInList(string $attributeKey, array $attributes): ?Document
Copy link
Member

Choose a reason for hiding this comment

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

This add unnecessary pollution to the API. This method should not be exposed to consumers of this library as it add unnecessary complexity, and noise.

We can move it to the MariaDB adapter as no other adapter uses it. This location can be reconsidered when we will need it in other places.

The method name doesn't seem to apply to any existing name pattern of the current API. I would vote to call it findAttribute and mark it protected.

* @return Document
* @throws Exception
*/
public function fixIndex(Document $index, array $attributes): Document {
Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't be part of the adapter interface as this issue is specific to MariaDB/MySQL.

Also, the name suggest this is a bug fix and might encourage future maintainer to use this as a generic place for adding patches that will increase technical debt.

This method should be designed for enforcing the required behavior for the relevant adapter. getSQLIndexLimit(int $length): int might better explain what we do, and work better with the existing patterns of getSQLIndex() and getSQLIndexType(). Very important we keep consistency and predictability across the library.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I Do not think this is relevant since now it moved to the MariaDB.
Please give me a better name for fixIndex
This function returns the index after checking and overwriting bad values

@abnegate
Copy link
Member

abnegate commented Jun 9, 2023

@fogelito Is this still relevant or is it solved by the index size validation?

@fogelito
Copy link
Contributor Author

Yes, I will close this one.
Was taken care by a validation here:
#271

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.

4 participants