Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add hasColumn/hasColumns method #3001

Merged
merged 9 commits into from
Jul 16, 2024
Merged

Add hasColumn/hasColumns method #3001

merged 9 commits into from
Jul 16, 2024

Conversation

Alex-Belyi
Copy link
Contributor

Add hasColumn/hasColumns method

Checklist

  • Add tests and ensure they pass
  • Add an entry to the CHANGELOG.md file
  • Update documentation for new features

@Alex-Belyi Alex-Belyi requested a review from a team as a code owner June 10, 2024 09:10
@Alex-Belyi Alex-Belyi requested a review from alcaeus June 10, 2024 09:10
{
return true;
Copy link
Member

@GromNaN GromNaN Jun 26, 2024

Choose a reason for hiding this comment

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

Technically, that's a breaking change as you change the return value of this method. MongoDB doesn't have columns and there is no restriction on field names (unless you use a schema validation). I understand that this method is not useful as it is, as it returns always true, but I don't think the proposed change is correct as it depends on the data in the collection. Which can lead to a different results depending on the data stored.

@alcaeus
Copy link
Member

alcaeus commented Jun 27, 2024

I agree with @GromNaN here. This method is essentially useless, because you're looking for "does any document in this collection have a field with this name". This is not something you should be checking for, and is also an extremely inefficient query as it's usually run unindexed, and thus relies on a collection scan and BSON inspection of every document in the collection.

If there's a specific use case you'd like to change this method for, please share this use case with us, but I don't see any benefit in changing the current implementation.

@alcaeus alcaeus closed this Jun 27, 2024
@Alex-Belyi
Copy link
Contributor Author

Alex-Belyi commented Jul 3, 2024

@GromNaN @alcaeus
Sorry for late answer.

I want to share example why I added such code.

So I have bitbucket pipelines configured to deploy project to virtual machine
There is three options for DB (keep existing, reset all, reset only mysql (keep mongo)).

We use single Laravel project which works with migrations (mysql + mongo)

So there an issue if went to keep mongo DB and reset only mysql

    public function up(): void
    {
        Schema::connection('mongodb')->table('table_name', function (Blueprint $collection) {
            $collection->unique('id');
        });
    }
        

Will cause an error about existing unique key

so I added some manual 'hasColumn' method to avoid such situation

    public function up(): void
    {
        Schema::connection('mongodb')->table('table_name', function (Blueprint $collection) {
            if (MongoDb::hasColumn('table_name', 'id')) {
                $collection->unique('id');
            }
        });
    }

@GromNaN
Copy link
Member

GromNaN commented Jul 3, 2024

Thanks for taking the time to give a little more context.
It seems to me that this use-case does not correspond to the purpose of this method in Laravel.
Here are a few comments:

  • The fact that the index is created conditionally if there is data in the field or not, seems flaky.
  • The state of the database migrations is stored in the database (in a MySQL table and a MongoDB collection), you should be able to rely on the migrations to know the state of your database schema.
  • If you want to ignore the error, wrap the $collection->unique('id'); in a try...catch.

@dercoder
Copy link

@GromNaN So what is the purpose of this method then? I beleive that returning always true is also not the right purpose here.
Should this method not check if the column exists in the table or not?

I know, MongoDB is not using columns and also not tables but this package should handle MongoDB in this way, right?

So when you don't like this approach, what do you think about to have a seperate method here?

Schema::connection('mongodb')->table('table_name', function (Blueprint` $`collection) {
    if (Schema::hasField('table_name', 'myField')) {
        // My code ...
    }
});

You already added the hasCollection method into the Schema class so it would be similar to this.

Here I understand why @Alex-Belyi created this PR. As I remember, not all laravel features are implemented in this package.
If I am not wrong, when you do a fresh migration, the indexes in MongoDB are still saved into MongoDB.

Also this "try/catch" solution is not a real solution.
When there are duplicate values in the collection, the unique index will not be created at all.
Here it's hard to distinguish if a index already exists or an index can not be created because of duplicate entities.

Another issue can be that the migration table only exists in MySQL and not in MongoDB.
It would be nice to have some option to use a migration table per instance (one for MySQL and one for MongoDB).
Only then developers can rely on the migrations but I beleive this solution is not so easy to implement here. @GromNaN Do you think this would be possible?

@GromNaN
Copy link
Member

GromNaN commented Jul 16, 2024

Reopening. This feature would make sense as we just implemented getColumns with a similar mechanism #3045

@GromNaN GromNaN reopened this Jul 16, 2024
@GromNaN GromNaN changed the base branch from 4.5 to 4.7 July 16, 2024 15:34
@GromNaN GromNaN enabled auto-merge (squash) July 16, 2024 17:06
@GromNaN
Copy link
Member

GromNaN commented Jul 16, 2024

Thank you @Alex-Belyi

@GromNaN GromNaN merged commit a62d4b9 into mongodb:4.7 Jul 16, 2024
26 checks passed
@GromNaN GromNaN added this to the 4.7 milestone Jul 19, 2024
rustagir pushed a commit to rustagir/laravel-mongodb that referenced this pull request Jul 23, 2024
Co-authored-by: Jérôme Tamarelle <jerome.tamarelle@mongodb.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants