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

Loosen column length constraints #152

Merged
merged 3 commits into from Oct 14, 2018

Conversation

Projects
None yet
4 participants
@orien
Member

orien commented Oct 3, 2018

Resolves #145

Context

The default database schema provided by DoubleEntry had small column length limits. These values were specific to the optimisations made on a specific project at Envato. As this project is open source and in use under vastly different use cases, these constraints limit the project effectiveness at satisfying these use cases.

Change

  1. Remove the column length constraints from the database migration template. DoubleEntry will operate correctly no matter the size of these columns.

  2. Remove the default value for the scope_identifier_max_length, account_identifier_max_length and code_max_length configuration values. The checks will not be performed during account and transfer configuration unless these are options are set.

    DoubleEntry.configure do |config|
      config.code_max_length = 47
      config.account_identifier_max_length = 31
      config.scope_identifier_max_length = 23
    end

MySQL 5.6 schema diff on Rails 5.2 looks like this:

43,44c43,44
<   `account` varchar(31) NOT NULL,
<   `scope` varchar(23) DEFAULT NULL,
---
>   `account` varchar(255) NOT NULL,
>   `scope` varchar(255) DEFAULT NULL,
64,67c64,67
<   `account` varchar(31) NOT NULL,
<   `code` varchar(47) DEFAULT NULL,
<   `partner_account` varchar(31) DEFAULT NULL,
<   `scope` varchar(23) DEFAULT NULL,
---
>   `account` varchar(255) NOT NULL,
>   `code` varchar(255) DEFAULT NULL,
>   `partner_account` varchar(255) DEFAULT NULL,
>   `scope` varchar(255) DEFAULT NULL,
112,113c112,113
<   `key` varchar(48) NOT NULL,
<   `value` varchar(64) NOT NULL,
---
>   `key` varchar(255) NOT NULL,
>   `value` varchar(255) NOT NULL,
130,132c130,132
<   `account` varchar(31) NOT NULL,
<   `scope` varchar(23) DEFAULT NULL,
<   `code` varchar(47) NOT NULL,
---
>   `account` varchar(255) NOT NULL,
>   `scope` varchar(255) DEFAULT NULL,
>   `code` varchar(255) NOT NULL,
136,137c136,137
<   `partner_account` varchar(31) NOT NULL,
<   `partner_scope` varchar(23) DEFAULT NULL,
---
>   `partner_account` varchar(255) NOT NULL,
>   `partner_scope` varchar(255) DEFAULT NULL,

To apply this change a migration similar to the following could be used:

change_column :double_entry_account_balances, :account, :string, limit: 255, null: false
change_column :double_entry_account_balances, :scope,   :string, limit: 255, null: true

change_column :double_entry_line_aggregates, :account,         :string, limit: 255, null: false
change_column :double_entry_line_aggregates, :code,            :string, limit: 255, null: true
change_column :double_entry_line_aggregates, :partner_account, :string, limit: 255, null: true
change_column :double_entry_line_aggregates, :scope,           :string, limit: 255, null: true

change_column :double_entry_line_metadata, :key,   :string, limit: 255, null: false
change_column :double_entry_line_metadata, :value, :string, limit: 255, null: false

change_column :double_entry_lines, :account,         :string, limit: 255, null: false
change_column :double_entry_lines, :scope,           :string, limit: 255, null: true
change_column :double_entry_lines, :code,            :string, limit: 255, null: false
change_column :double_entry_lines, :partner_account, :string, limit: 255, null: false
change_column :double_entry_lines, :partner_scope,   :string, limit: 255, null: true

Engineering teams can choose to apply this change, or apply their own column length constraints specific to their needs.

@orien orien requested a review from envato/finance Oct 3, 2018

@scottyp-env

This comment has been minimized.

Show comment
Hide comment
@scottyp-env

scottyp-env Oct 3, 2018

LGTM, just got a question about the _max_length config values. Are they only there to fail fast so that values that are too long don't get inserted into the database and cause a blow up? Or do they have some other sort of business value? I guess I want to know, is it likely anyone is relying on those being set and they'll be upset if they haven't set those defaults in their config?

scottyp-env commented Oct 3, 2018

LGTM, just got a question about the _max_length config values. Are they only there to fail fast so that values that are too long don't get inserted into the database and cause a blow up? Or do they have some other sort of business value? I guess I want to know, is it likely anyone is relying on those being set and they'll be upset if they haven't set those defaults in their config?

@rizalmuthi

LGTM

@jhilde

This comment has been minimized.

Show comment
Hide comment
@jhilde

jhilde Oct 5, 2018

Ironically you did this PR just after we started talking about migrating to uuids.

Any idea when you’ll merge?

jhilde commented Oct 5, 2018

Ironically you did this PR just after we started talking about migrating to uuids.

Any idea when you’ll merge?

@orien

This comment has been minimized.

Show comment
Hide comment
@orien

orien Oct 11, 2018

Member

Hi @jhilde, I was hoping to get this merged this week.

Member

orien commented Oct 11, 2018

Hi @jhilde, I was hoping to get this merged this week.

@orien

This comment has been minimized.

Show comment
Hide comment
@orien

orien Oct 11, 2018

Member

just got a question about the _max_length config values. Are they only there to fail fast so that values that are too long don't get inserted into the database and cause a blow up?

@scottyp-env: That's exactly it. Back in Rails 3 when we inserted strings too large for the columns, the values would be silently truncated. So we added these checks.

Now Rails will raise an error when inserting such a string. So these checks are a nice to have as they'll alert us earlier in the development lifecycle, when defining the accounts and transfers rather than when the systems in operating in production.

Actually, now that I think about it the scope_identifier_max_length isn't checked until a transfer is made. There's little value in this check. I'll investigate removing it.

Member

orien commented Oct 11, 2018

just got a question about the _max_length config values. Are they only there to fail fast so that values that are too long don't get inserted into the database and cause a blow up?

@scottyp-env: That's exactly it. Back in Rails 3 when we inserted strings too large for the columns, the values would be silently truncated. So we added these checks.

Now Rails will raise an error when inserting such a string. So these checks are a nice to have as they'll alert us earlier in the development lifecycle, when defining the accounts and transfers rather than when the systems in operating in production.

Actually, now that I think about it the scope_identifier_max_length isn't checked until a transfer is made. There's little value in this check. I'll investigate removing it.

@orien

This comment has been minimized.

Show comment
Hide comment
@orien

orien Oct 14, 2018

Member

There doesn't seem a strong reason for the scope_identifier_max_length check. But I won't remove it here.

Member

orien commented Oct 14, 2018

There doesn't seem a strong reason for the scope_identifier_max_length check. But I won't remove it here.

@orien orien merged commit 9c8ea38 into master Oct 14, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@orien orien deleted the loosen_column_constraints branch Oct 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment