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

Add Support for ULID column type #657

Merged
merged 38 commits into from
Dec 12, 2023

Conversation

rcrosbourne
Copy link
Contributor

List of changes:

  1. Refactor foreign key generation in migrations
  2. Refactored the foreign key generation code in migration files into a method.
  3. Supports generation of primary keys with 'ulid' and 'uuid' data types for pivot tables.
  4. Added support for ULID and UUID in pivot tables migration
  5. Expanded the migration generation function to support ULID and UUID primary keys.
  6. Add ULID support in Model and ModelGenerator classes
  7. Function to check if ULIDs are used in the Model class.
  8. Expand 'ModelGenerator' class to act accordingly if ULIDs are used.
  9. Add support for nullableUlidMorphs and ulidMorphs column types
  10. The column types have been added to the all-column-types test fixtures and model generator.
  11. Update Foreign Key Constraints and Migration Generator
  12. Foreign key constraints updated in model test fixtures and new foreign keys customer_id and tran_id added.
    1. ulidMorphs added to the test fixtures in all-column-types for drafts, models, and factories.
  13. Add test for ULID without relationship scenario
  14. A new test for the case of a ULID without a relationship was added in MigrationGeneratorTest.php file.
  15. Add ULID support to FactoryGenerator and tests
  16. Add support for ULID shorthand in migrations
  17. Add ulid type to ModelLexer.php.
  18. Update regular expression in Blueprint.php to match ulid and uuid.

rcrosbourne and others added 30 commits December 5, 2023 07:56
The change updates the regular expression in Blueprint.php to match 'ulid' and 'uuid' both. The update will reflect in the functionality where these matches are used for id creation.
A ULID shorthand has been introduced to the MigrationGeneratorTest, allowing ULID fields to be easily added through drafts. This is captured in new tests and the corresponding fixture files, validating its successful integration.
ULID column type has been added to the FactoryGenerator and corresponding tests. This update augments the list of column data types that can be used, particularly aiding situations where a universally unique lexicographically sortable identifier (ULID) is required. Test fixtures are updated to validate this new addition.
The ULID column type has been added to the 'all-column-types' test fixture.
Two new files were created to handle the case of a ulid shorthand with an invalid relationship. This includes a test case in the MigrationGeneratorTest.php file and a matching migration file. A draft file for ulid-shorthand-invalid-relationship was also added to the fixtures folder.
A new test for the case of a ULID without a relationship was added in MigrationGeneratorTest.php file. Along with this, corresponding migration and draft files were created under the fixtures directory. This update aids in handling scenarios with ULID that do not have any specified relationships.
'ulidMorphs' was added to the test fixtures in all-column-types for drafts, models, and factories.
Foreign key constraints updated in model test fixtures and new foreign keys 'customer_id' and 'tran_id' added. Migration Generator has been modified to handle ULID datatype along with ID and UUID. This commit adapts the Migration Generator to recognize and handle the ULID datatype and updates tests to reflect this change.
A new function, `it_parses_ulid_shorthand`, is added to BlueprintTest to ensure correct parsing of ULID shorthand. Accordingly, assertions have also been updated to reflect this new generation method in the relevant test fixtures and new keys handling.
The `nullableUlidMorphs` and `ulidMorphs` column types have been added to the `all-column-types` test fixtures and model generator. The appropriate handling for these new column types were added to the `ModelGenerator` PHP class. This addition ensures that these column types are properly processed and reflected in the models.
Added a function to check if ULIDs are used in the 'Model' class. Expanded 'ModelGenerator' class to act accordingly if ULIDs are used. Added respective test case and fixture files to ensure correct functionality.
Added a function to check if ULIDs are used in the 'Model' class. Expanded 'ModelGenerator' class to act accordingly if ULIDs are used. Added respective test case and fixture files to ensure correct functionality.
…/blueprint into feature/add-ulid-column-type
This commit expands the migration generation function to support ULID and UUID primary keys, specifically in the context of pivot tables. Tests have been created for this new functionality, and related migration stubs have been updated. This allows the creation of pivot tables with ULID and UUID references, catering to a broader range of database design choices.
The foreign key generation code in migration files has been refactored into a separate method. This update simplifies the main function by extracting complex logic and improves readability of the code. Additionally, the method now supports generation of primary keys with 'ulid' and 'uuid' data types for pivot tables.
@jasonmccreary
Copy link
Collaborator

Sorry, this commit is way too large with too many commits and changes for me to consider. If you want a new feature added, I recommend separating your PR into multiple. First making the feature, then additional for refactors.

@rcrosbourne
Copy link
Contributor Author

Thanks for your feedback. This feature involved addingulid support for models, migrations and factories. So there would be a lot of files changes. I can take a phased approach by adding support for migrations only and slowly add the rest.

Copy link
Collaborator

@jasonmccreary jasonmccreary left a comment

Choose a reason for hiding this comment

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

Okay, I gave it a quick review. Please fix the comments and run vendor/bin/pint locally.

$this->assertEquals([
'models' => [
'Person' => [
'id' => 'ulid primary',
Copy link
Collaborator

Choose a reason for hiding this comment

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

It should not be required to say ulid primary, just ulid is fine as it is the conventional id field.

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 isn't required to specify ulid primary. Here is the draft file that was used in that test tests/fixtures/drafts/ulid-shorthand.yaml

Copy link
Collaborator

Choose a reason for hiding this comment

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

Since it is not needed, please remove ulid primary from all of your new fixtures to avoid confusion for those reviewing them as examples.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure why the linter is failing in the pipeline. It passes lint checks when I run it locally.

Screenshot 2023-12-10 at 15 58 12

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

src/Generators/MigrationGenerator.php Show resolved Hide resolved
src/Generators/MigrationGenerator.php Outdated Show resolved Hide resolved
src/Generators/MigrationGenerator.php Show resolved Hide resolved
@jasonmccreary
Copy link
Collaborator

Please resolve any open conversation and I'll give this a final review.

@jasonmccreary
Copy link
Collaborator

Looks like there's still a bit of lint. Run composer update locally and then vendor/bin/pint. It's probably a new rule in Pint that's not being applied.

@rcrosbourne
Copy link
Contributor Author

Looks like there's still a bit of lint. Run composer update locally and then vendor/bin/pint. It's probably a new rule in Pint that's not being applied.

Yes that worked. Thanks.

@jasonmccreary jasonmccreary merged commit 1431224 into laravel-shift:master Dec 12, 2023
8 checks passed
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