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.

Sign up for GitHub

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

Jump to bottom

feat(c14n): add canonicalization functionality #37

Merged
merged 39 commits into from
Dec 16, 2024
Merged

feat(c14n): add canonicalization functionality #37

merged 39 commits into from
Dec 16, 2024

Conversation

Tieske
Copy link
Contributor

@Tieske Tieske commented Nov 21, 2024 •
edited by kou
Loading

GitHub: fix GH-36

kou
kou reviewed Nov 22, 2024
View reviewed changes
xmlua/c14n.lua Outdated
local ffi = require("ffi")



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

@kou
Copy link
Member

kou commented Nov 22, 2024

Wow! I didn't know that libxml2 provides the c14n API.

Tieske
Tieske commented Nov 25, 2024
View reviewed changes
Copy link
Contributor Author

@Tieske Tieske left a comment

Choose a reason for hiding this comment

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

@kou thx for the review. Updated the PR. please have another look.

@Tieske Tieske mentioned this pull request Nov 25, 2024
Closed
@Tieske Tieske marked this pull request as ready for review November 25, 2024 14:26
@Tieske Tieske mentioned this pull request Nov 26, 2024
Closed
@Tieske Tieske force-pushed the c14n branch 2 times, most recently from 250e088 to e57e61d Compare November 27, 2024 06:05
kou
kou reviewed Nov 27, 2024
View reviewed changes
@Tieske
Copy link
Contributor Author

Tieske commented Nov 27, 2024

For the CI failures, not sure what happened. Since the failures are unrelated to this PR, it reminds me of the ffi-reloading issue. The test frameworks tend to isolate tests, by clearing the environment and run the next test clean. However the LuaJIT ffi is not suited for that and will occasionally segfault if you do.

See lunarmodules/busted#369 (comment)

kou
kou reviewed Nov 29, 2024
View reviewed changes
kou
kou reviewed Nov 29, 2024
View reviewed changes
@Tieske
Copy link
Contributor Author

Tieske commented Nov 30, 2024

I reworked your review comments one-by-one, a commit per comment. Just the last commit (merge into document.lua) is a big one that obfuscates all other changes. So probably best to review the changes on a per commit basis.

kou
kou reviewed Dec 4, 2024
View reviewed changes
@kou
Copy link
Member

kou commented Dec 4, 2024

Could you add some tests for document:canonicalize?

@Tieske
Copy link
Contributor Author

Tieske commented Dec 4, 2024

Could you add some tests for document:canonicalize?

yes it should, but my priorities are shifting, so might take a while.

@Tieske
Copy link
Contributor Author

Tieske commented Dec 4, 2024

basic test added

@Tieske
Copy link
Contributor Author

Tieske commented Dec 10, 2024

@kou any update on this?

kou
kou reviewed Dec 10, 2024
View reviewed changes
test/test-document-c14n.lua Outdated
Comment on lines 7 to 10
-- From: https://www.w3.org/2008/xmlsec/Drafts/c14n-20/test-cases/Overview.src.html

function TestDocumentC14N.test_PIs_Comments_and_Outside_of_Document_Element()
-- https://www.w3.org/2008/xmlsec/Drafts/c14n-20/test-cases/files/inC14N1.xml
Copy link
Member

@kou kou Dec 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Ah, we don't need to test C14N behaviors implemented in libxml2. Because they must be tested in libxml2.

We only need to test our API. For example, document:canonicalize(function) case, document:canonicalize(nodes) case, document:canonicalize(..., options) case and so on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agreed, all I did was just grab some of them to verify it works as intended.
This tests with and without "with_comments" probably also needs a test with the prefix list

@kou
Copy link
Member

kou commented Dec 10, 2024 •
edited
Loading

Sorry for my late response...

How about the followings to proceed this?

  1. Can we remove the PrefixToken format parsing feature from this PR and discuss it as a follow-up task? feat(c14n): add canonicalization functionality #37 (comment)
  2. Can I write tests for document:canonicalize() and push to this branch? (Or could you update tests?) feat(c14n): add canonicalization functionality #37 (comment)
  3. Can we merge this after 1. and 2.?

(We can write documents for this later.)

@Tieske
Copy link
Contributor Author

Tieske commented Dec 11, 2024

Sorry for my late response...

How about the followings to proceed this?

1. Can we remove the `PrefixToken` format parsing feature from this PR and discuss it as a follow-up task? [feat(c14n): add canonicalization functionality #37 (comment)](https://github.com/clear-code/xmlua/pull/37#discussion_r1878857950)

Done

2. Can I write tests for `document:canonicalize()` and push to this branch? (Or could you update tests?) [feat(c14n): add canonicalization functionality #37 (comment)](https://github.com/clear-code/xmlua/pull/37#discussion_r1878862733)

3. Can we merge this after 1. and 2.?

if you handle item 2, then 3 is also yours to handle. So please go ahead.

@kou
Copy link
Member

kou commented Dec 16, 2024

I've fixed test failures on macOS on master and rebased on master.

I've added tests. I also added some changes:

  • Removed C14N_ prefix from mode value because it's redundant in the document:canonicalize() context.
  • Fixed a bug that document:canonicalize({node}) doesn't work.
  • Improved some names to adjust other codes.

I'll merge this if you don't object these changes.

@Tieske
Copy link
Contributor Author

Tieske commented Dec 16, 2024

Lgtm!

@kou kou merged commit 98cf9ae into clear-code:master Dec 16, 2024
2 checks passed
@kou kou mentioned this pull request Dec 16, 2024
Open
@kou
Copy link
Member

kou commented Dec 16, 2024

Merged!

Should we release the current master as a new version? Or should we release a new version after #40?

@Tieske Tieske deleted the c14n branch December 17, 2024 14:04
@Tieske
Copy link
Contributor Author

Tieske commented Dec 17, 2024

no real opinion here. Since that would add a feature, it can be done at any later time as well.

@kou
Copy link
Member

kou commented Dec 17, 2024

OK. I'll release a new version before #40.

@kou
Copy link
Member

kou commented Dec 17, 2024

Released.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kou kou kou left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Q: how to include namespaces on serialized child element
2 participants
@Tieske @kou