Skip to content

Conversation

@UseTheFork
Copy link
Collaborator

@UseTheFork UseTheFork commented Aug 3, 2024

This commit introduces several new features and improvements to the package:

  • Nix Integration: Includes formatters, pre-commit hooks, and general environment setup.
  • Orchestra/Testbench: Integrates Testbench into the package.
  • Seeders and Factories Conversion: Converts all seeders and factories from laravel-elasticsearch-tests.
  • Base Testing Suite: Creates a base testing suite based on the site documentation.
  • Independent Tests: Modifies all tests to run 100% independently of each other (tests should never rely on each other).
  • GitHub Action for CI/CD: Adds a GitHub action to test for CI/CD (this will need further work but needs to be merged first).

There is definitely work to be done with coverage but in general this is a solid foundation:
image

…es for User, UserProfile, Client, ClientLog, ClientProfile, BlogPost, Company, and CompanyLog
- Enabled PHPStan for code linting to enhance code quality checks.
- Updated `flake.nix` to include PHPStan in the CI pipeline.
@pdphilip
Copy link
Owner

Pint, did add it to composer dev.

Tho I saw some weird formatting, but I think that was when I was switching branches and an outdated gitignore caused chaos.

Anyway. Pint good sir

@UseTheFork
Copy link
Collaborator Author

Pint, did add it to composer dev.

Tho I saw some weird formatting, but I think that was when I was switching branches and an outdated gitignore caused chaos.

Anyway. Pint good sir

Got it. I'm using PHP CS Fixer. I will switch that off and add Pint.

This is all looking really good I think. I get how the package works now So its getting easier to add features. I am also using tests from the MongoDB package and converting them to work with laravel-elasticsearch.

@pdphilip
Copy link
Owner

Super. Let's park any new features for now. I really want to ship this and use it as the base going forward. Great work

@UseTheFork
Copy link
Collaborator Author

Super. Let's park any new features for now. I really want to ship this and use it as the base going forward. Great work

Agreed! I will make a new branch on my repo for anything else I am doing and merge in as separate PR after this goes in.

@pdphilip
Copy link
Owner

pdphilip commented Sep 2, 2024

Update:

I have made a few changes to the bulk method:

  1. I set chunk size to 1000 at a time (as done in PHP ES docs example), this feels safe for memory considerations

  2. The default is that it returns a summary array of what happened, ex:

{
"success": true,
"timed_out": false,
"took": 5082,
"total": 100000,
"created": 100000,
"modified": 0,
"failed": 0
}

(That's a real result by the way, amazing that it can create 100k records in 5sec)

If there are any errors, it will return the details in an error field:

{
"success": false,
"timed_out": false,
"took": 5055,
"total": 100000,
"created": 99619,
"modified": 0,
"failed": 381,
"error": [....], //<-errors in here
"errorMessage": "Bulk insert failed for some values"
}

It also differentiates between created/modified, ex:

$prods = Product::limit(100)->get();
return Product::insert($prods->toArray());
{
"success": true,
"timed_out": false,
"took": 7,
"total": 100,
"created": 0,
"modified": 100,
"failed": 0
}

insert has a second parameter if you want the [successful] data returned

$prods = Product::limit(100)->get();
$updatedProds = Product::insert($prods->toArray(),true);
return $updatedProds; //shows the data, not the summary

The reason that this is not the default is that at mass this will exhaust most memory settings

Another major upgrade is that get(), search() and insert() returns an ElasticCollection which has the metadata embedded in it. It works like a normal collection but you get access to the query meta

$prods = Product::limit(100)->get();
$updatedProds = Product::insert($prods->toArray(),true);
return $updatedProds->getQueryMetaAsArray();
$prods = Product::where('orders','>=',100)->get();
$prods->getDsl();
$prods->getQueryMetaAsArray();

Let's peg it here now, then finish up the last of the tests and get this shipped

@UseTheFork
Copy link
Collaborator Author

@pdphilip Yea, this all makes sense to me, and I thought about doing something similar. I think your approach works well. What other tests do we need before this can get merged in?

@pdphilip
Copy link
Owner

pdphilip commented Sep 2, 2024

I guess just the toDo()s - I've tested all the changes with my existing Test Suite covering 207 tests, though those don't include the cursor and bulk tests. Anyway, I'll wrap up the current toDo() ones, can you do a test for bulk?

Then we go.

@UseTheFork
Copy link
Collaborator Author

@pdphilip Is there a reason you removed:

        if ($refresh) {
            $params['refresh'] = $refresh;
        }

With out it it never waits for a refresh so it ends up failing tests since it's still building the records. I am going to add it back in since that way we can add insertWithOutRefresh in the future.

@pdphilip
Copy link
Owner

pdphilip commented Sep 2, 2024

Go for it

@UseTheFork
Copy link
Collaborator Author

Go for it

with that back in it's working perfect! I am going to add a test to make sure it's returning a ElasticCollection and after that I think you can ship.

- Added optional `refresh` parameter to `insertBulk` method in `Connection.php`.
- Updated `processInsertBulk` method in `DSL/Bridge.php` to handle the new parameter.
- Modified `insertBulk` call in `Query/Builder.php` to include `refresh` parameter.
- Implemented a new test in `Eloquent/InsertTest.php` to verify the functionality.
@UseTheFork
Copy link
Collaborator Author

@pdphilip This is ready for you!

UseTheFork and others added 5 commits September 2, 2024 17:58
- Introduced rootDir variable for consistency.
- Added Laravel Pint formatter configuration to treefmt.
- Removed redundant php-cs-fixer setup.
- Disabled phpstan linting.
@pdphilip
Copy link
Owner

pdphilip commented Sep 3, 2024

@use-the-fork your CI is excellent, tested it and works great. Well done.

I'm going to leave the todo tests for now as is and prepare this release (update docs etc) - that's always a bit of admin. Then we ship this beast.

This is an amazing contribution, thanks a bunch. And I hope you'll stick around for more.

@pdphilip
Copy link
Owner

pdphilip commented Sep 3, 2024

Also, if you'd like to be in touch, shoot me an email (in composer)

@UseTheFork
Copy link
Collaborator Author

@use-the-fork your CI is excellent, tested it and works great. Well done.

I'm going to leave the todo tests for now as is and prepare this release (update docs etc) - that's always a bit of admin. Then we ship this beast.

This is an amazing contribution, thanks a bunch. And I hope you'll stick around for more.

Yeah, some of the TODO tests require code additions that will make this PR to be even larger. I was just leaving them as placeholders until this gets merged in, and I can create smaller, easier-to-review PRs.

The CI is also not done I needed it merged into the main so I can modify it and test it in my branch. Github is weird and if its not in the main branch I can't test any changes :(

I'm not going anywhere I use this package all the time easily one of the most useful packages I have ever found so I'm excited to contribute!

@pdphilip pdphilip merged commit 9c90f2e into pdphilip:main Sep 4, 2024
@UseTheFork UseTheFork deleted the refract/cleanup branch September 7, 2024 12:53
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.

3 participants