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 Secret base type #8519

Merged

Conversation

conradogarciaberrotaran
Copy link
Contributor

@conradogarciaberrotaran conradogarciaberrotaran commented Jan 9, 2024

Change Summary

Related issue number

Closes #8441

Checklist

  • The pull request title is a good summary of the changes - it will be used in the changelog
  • Unit tests for the changes exist
  • Tests pass on CI
  • Documentation reflects the changes where applicable
  • My PR is ready to review, please add a comment including the phrase "please review" to assign reviewers

Selected Reviewer: @sydney-runkle

Copy link

codspeed-hq bot commented Jan 9, 2024

CodSpeed Performance Report

Merging #8519 will not alter performance

Comparing conradogarciaberrotaran:add-secret-date-field (253a5a2) with main (b2aa36a)

Summary

✅ 10 untouched benchmarks

@sydney-runkle
Copy link
Member

@conradogarciaberrotaran,

Thanks for your work on this! Looking great thus far. Just curious - are there any other types that you'd like to see SecretX versions of?

The tests look great 🚀.

Now that we have more than just 2 SecretX types (used to just be SecretStr and SecretBytes), I think it makes sense to change the _SecretField class to more cleanly support a variety of secret types. I need to think a bit more about what the best pattern for this is.

Would it be ok if I pull your branch down and try a few variations on the code that you've added thus far?

@conradogarciaberrotaran
Copy link
Contributor Author

conradogarciaberrotaran commented Jan 18, 2024

Sure! This was a quick implementation I've tried. I'm not yet fully familiar with V2 way of defining new types.
I was thinking that we could make it a kwarg of Field, as in

foo: str = Field(secret=True)
SecretStr = Annotate[str, Field(secret=True]

what do you think?

@sydney-runkle
Copy link
Member

@conradogarciaberrotaran,

Great, I'll pull it down soon :).

I can see the argument for that, though I think it spirals in that we'd have to support secret behavior for all types in that case. I think for now we'll probably want to stick with explicitly defined secret types.

@conradogarciaberrotaran
Copy link
Contributor Author

Makes sense, let me know how can I assist, I'd really like to contribute with Pydantic :)

@sydney-runkle
Copy link
Member

@conradogarciaberrotaran,

Sure thing! Alright, I've pushed some changes in regards to how we inherit from _SecretField.

Currently, we don't support the validation of SecretX types on their own. For example:

from pydantic import SecretStr, TypeAdapter, ValidationError

s_invalid = SecretStr(123)
print(f"type: {type(s_invalid.get_secret_value())}, value: {s_invalid.get_secret_value()}")
#> type: <class 'int'>, value: 123

try:
    ta = TypeAdapter(SecretStr)
    s_validated = ta.validate_python(123)
except ValidationError as e:
    print(f"ValidationError: {e}")
    """
    ValidationError: 1 validation error for json-or-python[json=function-after[SecretStr(), str],python=union[is-instance[SecretStr],function-after[SecretStr(), str]]]
    Input should be a valid string [type=string_type, input_value=123, input_type=int]
        For further information visit https://errors.pydantic.dev/2.6/v/string_type
    """

I've removed the support for this for SecretDate. I've also modified a few of the tests that are failing as a consequence of this removed support.

I think for now, we should stick to not supporting that behavior, as implementation would require some complex changes (I don't think we'd want to import the SchemaValidator directly).

As a side note, this issue is related to the above discussion: This issue is related: #7353.

@sydney-runkle
Copy link
Member

@conradogarciaberrotaran,

I'll make sure everything is working in terms of linting / testing, then hand this back to you :).

@sydney-runkle
Copy link
Member

sydney-runkle commented Jan 18, 2024

@conradogarciaberrotaran,

Makes sense, let me know how can I assist, I'd really like to contribute with Pydantic :)

Could you please update this example with SecretDate functionality? https://docs.pydantic.dev/latest/examples/secrets/#serialize-secretstr-and-secretbytes-as-plain-text

Other than that, I think this PR is ready for review, so I'll mark it as such. I've labeled many of our other issues as help wanted if you're interested in tackling another task!

Thanks a bunch for your help with this - we're super excited to adopt this feature :).

@sydney-runkle sydney-runkle marked this pull request as ready for review January 18, 2024 16:10
@sydney-runkle sydney-runkle changed the title WIP SecretDate Support SecretDate type Jan 18, 2024
@sydney-runkle
Copy link
Member

Please review

@conradogarciaberrotaran
Copy link
Contributor Author

Thanks! I'll take a look at the docs and will probably try to make a secret Enum!

@sydney-runkle
Copy link
Member

Thanks! I'll take a look at the docs and will probably try to make a secret Enum!

Sounds great! Ping me for a review when you're done :).

Copy link
Member

@samuelcolvin samuelcolvin left a comment

Choose a reason for hiding this comment

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

idea looks good, I wonder if we're be better to implement this via

SecretDate = Annotated[date, Secret(repr=lambda value: '****/**/**')]

So we can easily add more types, even if we can't use the Secret type for SecretStr and SecretBytes until V3 for compatibility reasons.

pydantic/types.py Outdated Show resolved Hide resolved
pydantic/types.py Outdated Show resolved Hide resolved
pydantic/types.py Outdated Show resolved Hide resolved
pydantic/types.py Outdated Show resolved Hide resolved
pydantic/types.py Outdated Show resolved Hide resolved
@sydney-runkle
Copy link
Member

idea looks good, I wonder if we're be better to implement this via

SecretDate = Annotated[date, Secret(repr=lambda value: '****/**/**')]

So we can easily add more types, even if we can't use the Secret type for SecretStr and SecretBytes until V3 for compatibility reasons.

Agreed. @conradogarciaberrotaran, are you interested in adding this type, or would you like me to? I can also help you get a start if you need 👍.

Also, thanks for making the changes that @samuelcolvin requested. Your changes look good.

@conradogarciaberrotaran
Copy link
Contributor Author

Just pushed missing tests and doc updates!
let me know if we're missing something else

@conradogarciaberrotaran
Copy link
Contributor Author

please review

@conradogarciaberrotaran conradogarciaberrotaran changed the title Support SecretDate type Add Secret base type Feb 4, 2024
@sydney-runkle
Copy link
Member

Update, I don't think this is required for this PR:

 Figure out how to support Strict() specification on Secret types directly, now that we're not using the clunky Union schema

It'd be a nice feature down the line, but you can currently apply Strict on the inner type, which is a good enough start!

@sydney-runkle
Copy link
Member

I believe all relevant concerns have been addressed here and this is ready for review :).

docs/examples/secrets.md Outdated Show resolved Hide resolved
docs/examples/secrets.md Outdated Show resolved Hide resolved
Copy link
Member

@adriangb adriangb left a comment

Choose a reason for hiding this comment

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

I haven’t reviewed the tests or implementation in detail but in general I really like this approach!

sydney-runkle and others added 4 commits February 8, 2024 17:34
Co-authored-by: David Montague <35119617+dmontagu@users.noreply.github.com>
Co-authored-by: David Montague <35119617+dmontagu@users.noreply.github.com>
@sydney-runkle sydney-runkle merged commit 1c91c86 into pydantic:main Feb 9, 2024
53 checks passed
@sydney-runkle
Copy link
Member

@conradogarciaberrotaran,

Nice work on helping implement this new feature! It'll be one of the highlights of our 2.7 release :).

@conradogarciaberrotaran
Copy link
Contributor Author

Thanks for your help! I hope I can contribute more in the future.

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

Successfully merging this pull request may close these issues.

SecretField other than str and bytes
5 participants