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

fix: a few Ruby DSL schema dump bug fixes #308

Merged
merged 5 commits into from
Aug 31, 2024

Conversation

jarredhawkins
Copy link
Contributor

@jarredhawkins jarredhawkins commented Aug 15, 2024

  1. Make sure array types are preserved in the dump
    Otherwise restoring from the dump, columns end up dropping the array type and coercing things to the base type

  2. Consistently sort storing data to avoid diffs in committed structure.rb file
    Right now it's dependent on the ordering of data from table_columns, which doesn't specify ordering.

@jarredhawkins jarredhawkins requested review from olavloite and a team as code owners August 15, 2024 01:16
Copy link

google-cla bot commented Aug 15, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@product-auto-label product-auto-label bot added the api: spanner Issues related to the googleapis/ruby-spanner-activerecord API. label Aug 15, 2024
@jarredhawkins jarredhawkins changed the title A couple of :ruby format schema dump bug fixes fix: a few Ruby DSL schema dump bug fixes Aug 15, 2024
Copy link

🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use automerge label. Good luck human!

-- conventional-commit-lint bot
https://conventionalcommits.org/

1. Make sure `array` types are preserved in the dump
    Otherwise restoring from the dump, columns end up dropping the `array` type and coercing things to the base type

1. Consistently sort `storing` data to avoid diffs in committed `structure` file
    Right now it's dependent on the ordering of data from `table_columns`, which doesn't specify ordering.
@@ -27,6 +27,8 @@ def prepare_column_options column
spec = { type: schema_type(column).inspect }.merge! spec
end

spec[:array] = true if column.sql_type.start_with?('ARRAY<')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

feels like there's probably a safer way to determine if a ActiveRecord::ConnectionAdapters::Spanner::Column is ARRAY type, deferring to reviewers here

Copy link
Collaborator

Choose a reason for hiding this comment

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

I had a look at what is available here, and there isn't really anything else that you could use without changing the underlying fetching of column metadata. That would make this change much bigger, and I don't think there's any real problem with just checking the type name like this.

@olavloite olavloite merged commit 961ee1e into googleapis:main Aug 31, 2024
32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner Issues related to the googleapis/ruby-spanner-activerecord API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants