Skip to content

The AccountBuilder can now skip None Values#7

Open
stop5 wants to merge 2 commits into
jamsocket:mainfrom
stop5:feature/better-account-builder
Open

The AccountBuilder can now skip None Values#7
stop5 wants to merge 2 commits into
jamsocket:mainfrom
stop5:feature/better-account-builder

Conversation

@stop5

@stop5 stop5 commented May 7, 2025

Copy link
Copy Markdown

By using an Serializer that takes care of the checks and transformations it converts the data into json for the authenticates_request method

By using an Serializer that takes care of the checks and transformations
it converts the data into json for the authenticates_request method
@stop5 stop5 marked this pull request as draft May 7, 2025 07:02
@stop5

stop5 commented May 7, 2025

Copy link
Copy Markdown
Author

I decided to write an own Serializer, because the normal serializer would consume the reference and the Type conversions would be better elsewhere

@paulgb paulgb requested a review from Copilot May 7, 2025 15:54

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull Request Overview

This PR updates the AccountBuilder to skip serializing None values by implementing a custom Serialize trait, and refactors the build method to ensure a private key is generated if one is not provided.

  • Implements custom Serialize for AccountBuilder to conditionally serialize fields
  • Refactors build() to generate and cache a private key if absent
  • Leverages serde_json::to_value for cleaner JSON payload generation

Comment thread src/account.rs Outdated
obj.serialize_entry("onlyReturnExisting", &self.only_return_existing.clone())?
}
if let Some(eab) = self.eab_config.clone() {
let payload = serde_json::to_string(&Jwk::new(&self.private_key.clone().unwrap())).unwrap();

Copilot AI May 7, 2025

Copy link

Choose a reason for hiding this comment

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

Serializing the external account binding relies on unwrapping the private_key, which could panic if it is None. Consider handling the error more gracefully or ensuring that private_key is always set prior to serialization.

Suggested change
let payload = serde_json::to_string(&Jwk::new(&self.private_key.clone().unwrap())).unwrap();
let private_key = match &self.private_key {
Some(key) => key,
None => {
return Err(serde::ser::Error::custom("private_key is not set, cannot serialize externalAccountBinding"));
}
};
let payload = match serde_json::to_string(&Jwk::new(private_key)) {
Ok(p) => p,
Err(error) => {
return Err(serde::ser::Error::custom(format!("Failed to serialize JWK: {}", error)));
}
};

Copilot uses AI. Check for mistakes.
@paulgb

paulgb commented May 7, 2025

Copy link
Copy Markdown
Member

Hi @stop5, thanks for the PR. Could you give a bit more context about why this is necessary? It's been a while since I read the ACME spec, is null not allowed for these?

I wonder if #[serde(skip_serializing_if = "Option::is_none")] would do the same thing here?

@stop5

stop5 commented May 7, 2025

Copy link
Copy Markdown
Author

The Biggest problem i had trying to do it with derive(Serializer) are the PKey in the AccountBuilder and the ExternalAccountBinding. When the ExternalAccountBinding is added, it needs both keys to convert it into the value required for the request. So i couldn't add it to the ExternalAccountBinding struct

@stop5 stop5 force-pushed the feature/better-account-builder branch from 1b7190e to ce7477a Compare May 7, 2025 17:17
@stop5 stop5 marked this pull request as ready for review May 9, 2025 13:35
@stop5

stop5 commented Jun 22, 2025

Copy link
Copy Markdown
Author

to show the issue here is my code: https://gitea.sebastian-tobie.de/sebastian/racme
With the existing code it causes problems, but with the the patches it works.

@paulgb

paulgb commented Jun 23, 2025

Copy link
Copy Markdown
Member

Thanks for that context, but that page is a 404 for me.

@stop5

stop5 commented Jun 24, 2025

Copy link
Copy Markdown
Author

i forgot that the repo was created by push. Its now publicly available.

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