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

Enhance Integer and Float Handling in Provider Set #61

Open
wants to merge 1 commit into
base: development
Choose a base branch
from

Conversation

bizzappdev
Copy link

Improvements for integers and floats and how they are managed within the provider set.

When the field's data type is Integer or Numeric, the set provider raises an error because the values are not properly typecasted.

This PR provides proper typecast for integer and float/numeric datatype fields.

@bizzappdev bizzappdev marked this pull request as ready for review May 27, 2024 07:20
@hkage
Copy link
Contributor

hkage commented May 31, 2024

Thanks for your contribution and this PR. I will take a look into your changes asap.

@hkage
Copy link
Contributor

hkage commented Jun 10, 2024

Can you give me an example when the error occurs (e.g. the table structure and YAML schema?) Does the error only occurs when you pass integer or float values with quotes within the YAML schema? Just to be able to reproduce the behavior.

In my testings I was able to set integer values, as long as I didn't quote them but couldn't set any numeric or float fields, no matter what branch I used (development or your pull request).

     Column      |          Type          | Collation | Nullable | Default 
-----------------+------------------------+-----------+----------+---------
 id              | bigint                 |           |          | 
 first_name      | character varying(250) |           | not null | 
 last_name       | character varying(250) |           | not null | 
 data            | jsonb                  |           |          | 
 metadata        | json                   |           |          | 
 position        | integer                |           |          | 
 price           | numeric                |           |          | 
 price_exclusive | double precision       |           |          | 
tables:
 - auth_user:
    primary_key: id
    chunk_size: 5000
    fields:
     - first_name:
        provider:
          name: fake.first_name
     - last_name:
        provider:
          name: set
          value: "Bar"
     - position:
         provider:
           name: set
           value: 1000
     - price:
         provider:
           name: set
           value: 90.99
     - price_exclusive:
         provider:
           name: set
           value: 200.99

Before the anonymization:

anonymization_test=# select first_name, last_name, position, price, price_exclusive from auth_user;
 first_name | last_name | position | price | price_exclusive 
------------+-----------+----------+-------+-----------------
 Daniel     | Bar       |      500 |       |                

After calling the anonymization:

 first_name | last_name | position | price | price_exclusive 
------------+-----------+----------+-------+-----------------
 Jennifer   | Bar       |     1000 |       |                

pganonymize/version.py Outdated Show resolved Hide resolved
@bizzappdev bizzappdev force-pushed the 0.11.0-imp-integer-float-in-provider-set-BAD branch from c02fa6f to c081bd2 Compare June 10, 2024 10:32
@hkage
Copy link
Contributor

hkage commented Jun 14, 2024

Can you give me just a small example how to force the error (so I can test and review this pull request)? Especially for the numeric types. As mentioned above I could only reproduce an error for integer types if I use double quotes.

@bizzappdev
Copy link
Author

I will prepare in 1 or 2 days and will share the details with you.

@fblackburn1
Copy link
Contributor

fblackburn1 commented Jul 17, 2024

Hi @bizzappdev from this comment, @hkage is waiting for this PR before preparing a new release. Do you think you will have time to address feedback soon? No pressure, I just want to know if it worth to request a release without this PR :)

@hkage
Copy link
Contributor

hkage commented Jul 17, 2024

Hi @bizzappdev from this comment, @hkage is waiting for this PR before preparing a new release. Do you think you will have time to address feedback soon? No pressure, I just want to know if it worth to request a release without this PR :)

I could prepare a new release, even if this PR is still open. But as long as I can't reproduce the bug, I cannot merge it.

My suggestion would be to create a new minor version and release this bugfix (as soon as I can test it) as a bugfix release afterwards.

@fblackburn1
Copy link
Contributor

My suggestion would be to create a new minor version and release this bugfix (as soon as I can test it) as a bugfix release afterwards.

Yes it will be nice, thank you 😃

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