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

Zach/support built in logical types #22

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

minionOfZuul
Copy link
Contributor

This PR:

  • adds support for all built-in avro logical types, including those that have arguments. The only logical type like this in ruby avro at the moment is decimal which requires precision and scale arguments. It adds this support in a future-proof way. See the README change in this PR for example usage.
  • changes the API slightly for the currently-supported logical types. Any current use of date, datetime, or time would need to be updated like this required :created_at, :time, avro: { logical_type: 'timestamp-millis' }. Because of this, I bumped the major version of the gem. It is probably possible to make this change in a backward compatible way. Let me know if that's desirable and I can do it. I made a list of affected projects at Acima by doing a code search. Each project just has a handful of updates it would have to make. contract-funding, lms, virtual-bank, and onboarding_models are affected.

Reasoning for this PR
I'm working on removing the field_struct_avro_schema dependency from merchant-portal. The only reason it depends on FSAS is because of onboarding_models. The onboarding_models gem does have important data validations in it, which are active model compatible, so the idea is to split those validation functions into modules that can be included in both FSAS-based models and in anything else that supports ActiveModel, like avromatic.

During this work, I discovered the :currency data type. I assume this data type was created to get around using the decimal logical type, which is supported everywhere. To support :currency the FSAS gem must do a special pre-serialization and post-deserialization conversion process where it multiplies or divides the value currently in the model by 100. This makes the shared use of validations which want to call to_s on those values impractical. E.g. if I use FSAS and call .errors.full_messages I might get something like "Amount must be greater than 25.76". But if I make that same .errors.full_messages call on a model not using FSAS, I will get something like "Amount must be greater than 2576".

The solution here is to ditch :currency and just use the built-in avro type that is meant for decimal encoding.

@acima-infosec-github-appsec
Copy link

Hi, I noticed this repo does not have all of the required topics applied to it. In order to organize repos at Acima, all repos need to have several topics that describe the nature of the repo. Please apply the topics as specified by the confluence article here: https://acima.atlassian.net/wiki/spaces/SEC/pages/313622545/Why+did+I+get+comments+on+my+PR

You are missing the following topics:

  • The data topic in the format of 'data-<data_type>
    Possible values are:
    data-non-prod
    data-prod-customer
    data-prod-systems
  • The type topic in the format of 'type-<data_type>
    Possible values are:
    type-service
    type-library
    type-sample
    type-other

@@ -157,7 +170,8 @@
first_name: 'John',
last_name: 'Max',
title: 'VP of Engineering',
language: 'Haskell'
language: 'Haskell',
password: 'password123!'

Check failure

Code scanning / CodeQL

Hard-coded credentials

This hardcoded value is [used as credentials](1).
{ first_name: 'John', last_name: 'Stalingrad', title: 'Developer', language: 'Ruby' },
{ first_name: 'Steve', last_name: 'Romanoff', title: 'Designer', language: 'In Design' }
{ first_name: 'John', last_name: 'Stalingrad', title: 'Developer', language: 'Ruby', password: 'rubyroxx' },
{ first_name: 'Steve', last_name: 'Romanoff', title: 'Designer', language: 'In Design', password: 'IHeartComputers' }

Check failure

Code scanning / CodeQL

Hard-coded credentials

This hardcoded value is [used as credentials](1).
@@ -31,7 +32,7 @@
end

let(:developer_attrs) do
employee_attrs.merge language: 'Haskell'
employee_attrs.merge language: 'Haskell', password: 'password123!'

Check failure

Code scanning / CodeQL

Hard-coded credentials

This hardcoded value is [used as credentials](1).
end

let(:dev2_attrs) do
{ first_name: 'Steve', last_name: 'Romanoff', title: 'Designer', language: 'In Design' }
{ first_name: 'Steve', last_name: 'Romanoff', title: 'Designer', language: 'In Design', password: 'IHeartComputers' }

Check failure

Code scanning / CodeQL

Hard-coded credentials

This hardcoded value is [used as credentials](1).
{ first_name: 'John', last_name: 'Stalingrad', title: 'Developer', language: 'Ruby' },
{ first_name: 'Steve', last_name: 'Romanoff', title: 'Designer', language: 'In Design' }
{ first_name: 'John', last_name: 'Stalingrad', title: 'Developer', language: 'Ruby', password: 'rubyroxx' },
{ first_name: 'Steve', last_name: 'Romanoff', title: 'Designer', language: 'In Design', password: 'IHeartComputers' }

Check failure

Code scanning / CodeQL

Hard-coded credentials

This hardcoded value is [used as credentials](1).
@aemadrid
Copy link
Contributor

The history of using currency was that decimal was not available on the ruby side whent he code was built. So I added :currency to deal with that problem. Now that decimal has been added we can go ahead and start using decimal but yes, we do need this gem to support the special fields and we will need to do the work of migrating the shemas to using a new field instead - not sure we migrate the field type safely.

@aemadrid
Copy link
Contributor

One change I'm seeing that I didn't expect, and could break things, is changing int for longs. Is that on purpose?

@minionOfZuul minionOfZuul force-pushed the zach/support_built_in_logical_types branch from a86c2a5 to 8047df4 Compare December 18, 2023 22:00
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