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

Feature/jer #187

Merged
merged 22 commits into from
Nov 11, 2023
Merged

Feature/jer #187

merged 22 commits into from
Nov 11, 2023

Conversation

6d7a
Copy link
Member

@6d7a 6d7a commented Oct 27, 2023

Adds support for JSON encoding rules

JER-support is implemented as a non-default feature, both in rasn as the feature jer, as well as in rasn-derive as the feature text-based-encodings (changes in rasn-derive will likely be needed for XER handling, too). Encoder and decoder are built upon serde_json.

Closes #188
#109

@XAMPPRocky
Copy link
Collaborator

Thank you for your PR!

I have two initial bits of feedback, the first is that I don't think this should be a feature. If we want to go for that approach we should do that separately and apply it uniformly to the other codecs.

The second is the use of serde. Since we don't benefit from the generic codec framework of serde, since we are a codec framework ourselves, wouldn't we be better off using a purpose built json library, like jzon?

@6d7a
Copy link
Member Author

6d7a commented Oct 28, 2023

jzon looks indeed promising, I overlooked it designing the JER implementation. The API is similar to serde_json so it won't take much effort ro refactor. Seeing that jzon doesn't have any further dependencies I don't see any harm in not making JER a feature. I'll follow up with an update.

macros/Cargo.toml Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@6d7a
Copy link
Member Author

6d7a commented Nov 5, 2023

@XAMPPRocky I replaced serde_json with jzon and moved the jer feature into the main codebase. I also made a small change to Codec in order to handle text-based encoding rules. Unfortunately, some fuzz tests fail under specific builds and I'm having a hard time figuring out why. Do you have an idea? Thanks!

Copy link
Collaborator

@XAMPPRocky XAMPPRocky left a comment

Choose a reason for hiding this comment

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

LGTM to me mostly, just a couple comments.

src/codec.rs Outdated Show resolved Hide resolved
src/codec.rs Outdated Show resolved Hide resolved
_t: crate::Tag,
_c: Constraints,
) -> Result<TeletexString, Self::Error> {
todo!()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why todo here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Currently we do not have any logic to convert between String and TeletexString or to validate a TeletexString's bytes and it doesn't seem to be a trivial addition to make. PER also has a todo!() in its Decoder impl, so I'd propose to tackle this issue in a separate PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just a note when out of curiosity looking this PR, Teletex standard has been withdrawn. Should we just delete the type? https://www.itu.int/rec/T-REC-T.61 https://en.wikipedia.org/wiki/ITU_T.61 and focus on other more used ones. Get back to it if someone really needs it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately it keeps sticking around 🥲 It is also used in rasn-pkix. However, I think we can afford to be lenient when it comes to the implementations in jer, xer, oer, or per, since AFAIK T.61 had already been revoked when they were conceived.

Copy link
Contributor

@Nicceboy Nicceboy Nov 9, 2023

Choose a reason for hiding this comment

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

Oh, I missed that one. It is mentioned in oer as (TeletexString, T61String) both, but I haven't faced any standard which actually uses the types yet (when explicitly using OER).

Copy link
Contributor

@Nicceboy Nicceboy Nov 9, 2023

Choose a reason for hiding this comment

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

_c: crate::types::Constraints,
_value: &crate::types::TeletexString,
) -> Result<Self::Ok, Self::Error> {
todo!()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as above

@XAMPPRocky
Copy link
Collaborator

Thank you for your PR!

@XAMPPRocky XAMPPRocky merged commit 29f20c1 into librasn:main Nov 11, 2023
65 checks passed
This was referenced Nov 11, 2023
This was referenced Nov 23, 2023
This was referenced Dec 4, 2023
@github-actions github-actions bot mentioned this pull request Mar 9, 2024
@github-actions github-actions bot mentioned this pull request Mar 21, 2024
@github-actions github-actions bot mentioned this pull request Apr 3, 2024
@6d7a 6d7a deleted the feature/jer branch April 16, 2024 09:26
@github-actions github-actions bot mentioned this pull request May 8, 2024
This was referenced May 21, 2024
This was referenced Jun 11, 2024
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.

None yet

3 participants