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 docs for ActiveSupport::EncryptedFile#read [ci-skip] #45570

Conversation

whyinzoo
Copy link
Contributor

Summary

Adding docs to ActiveSupport::EncryptedFile#read

Other Information

Found this on https://www.codetriage.com/

@@ -49,6 +49,9 @@ def key
read_env_key || read_key_file || handle_missing_key
end

# Read and return decrypted content
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# Read and return decrypted content
# Reads the file and returns the decrypted content.

@@ -49,6 +49,9 @@ def key
read_env_key || read_key_file || handle_missing_key
end

# Read and return decrypted content
#
# Raise <tt>MissingContentError</tt> if unable to decrypt content
Copy link
Member

Choose a reason for hiding this comment

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

This method can raise other errors as well. For example, if raise_if_missing_key is true and the key is missing then the key.nil? call will raise MissingKeyError. Plus any errors due to an incorrect key or wrong file contents.

Also, we can link MissingContentError to its own API documentation by omitting <tt>.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing that out. I'll add that and any additional errors I see in EncryptedFile to the documentation.
I also noticed that decrypt can raise ActiveSupport::MessageEncryptor::InvalidMessage, is it customary to also include that as part of documentation? Thanks.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, let's include ActiveSupport::MessageEncryptor::InvalidMessage too.

@jonathanhefner jonathanhefner changed the title Add docs for ActiveSupport::EncryptedFile#read Add docs for ActiveSupport::EncryptedFile#read [ci-skip] Jul 13, 2022
@whyinzoo whyinzoo force-pushed the whyinzoo-update-docs-ActiveSupport--EncryptedFile-read-for-pr branch 2 times, most recently from 236ec14 to eef6065 Compare July 14, 2022 04:21
# Raises:
# - MissingContentError if unable to decrypt content.
# - MissingKeyError if key is missing and <tt>raise_if_missing_key</tt> is true.
# - InvalidKeyLengthError if key length is invalid.
Copy link
Member

Choose a reason for hiding this comment

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

read does not raise InvalidKeyLengthError. (write / encrypt does.)

Suggested change
# - InvalidKeyLengthError if key length is invalid.

Comment on lines 55 to 56
# - MissingContentError if unable to decrypt content.
# - MissingKeyError if key is missing and <tt>raise_if_missing_key</tt> is true.
Copy link
Member

Choose a reason for hiding this comment

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

MissingContentError is primarily for when the encrypted file is missing, though it will be raised if the key is missing and a MissingKeyError has not already been raised. Therefore, let's put these in chronological order:

Suggested change
# - MissingContentError if unable to decrypt content.
# - MissingKeyError if key is missing and <tt>raise_if_missing_key</tt> is true.
# - MissingKeyError if the key is missing and +raise_if_missing_key+ is true.
# - MissingContentError if the encrypted file does not exist or otherwise if the key is missing.

@whyinzoo whyinzoo force-pushed the whyinzoo-update-docs-ActiveSupport--EncryptedFile-read-for-pr branch from eef6065 to d64367e Compare July 16, 2022 04:05
@jonathanhefner jonathanhefner merged commit 4e9fee9 into rails:main Jul 18, 2022
@jonathanhefner
Copy link
Member

jonathanhefner commented Jul 18, 2022

Thank you, @whyinzoo! 🎉

(Backported to 7-0-stable.)

jonathanhefner added a commit that referenced this pull request Jul 24, 2022
…pport--EncryptedFile-read-for-pr

Add docs for ActiveSupport::EncryptedFile#read [ci-skip]

(cherry picked from commit 4e9fee9)
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.

None yet

2 participants