pantoniou / libfyaml

Fully feature complete YAML parser and emitter, supporting the latest YAML spec and passing the full YAML testsuite.
MIT License
239 stars 73 forks source link

Bug: Library cannot delete nodes contrary to documentation #115

Closed hq6 closed 1 month ago

hq6 commented 1 month ago

This issue can be mitigated run applying this patch:

diff --git a/src/lib/fy-doc.c b/src/lib/fy-doc.c
index 0032ee4..de0fbc1 100644
--- a/src/lib/fy-doc.c
+++ b/src/lib/fy-doc.c
@@ -2224,8 +2224,8 @@ int fy_node_insert(struct fy_node *fyn_to, struct fy_node *fyn_from)
        fyd_error_check(fyd, fyn_parent->type != FYNT_SCALAR, err_out,
                    "Illegal scalar parent node type");

-       fyd_error_check(fyd, fyn_from, err_out,
-               "Illegal NULL source node");
+//     fyd_error_check(fyd, fyn_from, err_out,
+//             "Illegal NULL source node");

        if (fyn_parent->type == FYNT_MAPPING) {
            /* find mapping pair that contains the `to` node */
pantoniou commented 1 month ago

Err, what is an example source snippet where this happens?

hq6 commented 1 month ago

Consider the following YAML file:

apiVersion: apps/v1
kind: Deployment
metadata:
  annotations:
    x: 2024-06-12t14:50:43z

Consider the following C source file:

#include <stdlib.h>
#include <stdio.h>

#include <libfyaml.h>

int main(int argc, char *argv[])
{
    struct fy_document *fyd = NULL;
  fyd = fy_document_build_from_file(NULL, argv[1]);
  fy_document_insert_at(fyd, "/metadata/annotations/x", FY_NT, NULL);
  fy_emit_document_to_fp(fyd, FYECF_DEFAULT, stdout);
}

Here is the result when I run it without the patch:

~/Code/YamlPlay main * ./MWE MWE.yaml
[ERR]: Illegal NULL source node
apiVersion: apps/v1
kind: Deployment
metadata:
  annotations:
    x: 2024-06-12t14:50:43z

Here is the result when I run it with the patch:

~/Code/YamlPlay main * ./MWE MWE.yaml
apiVersion: apps/v1
kind: Deployment
metadata:
  annotations: {}

The documentation for that function claims the following:

Insert a node to a given point in the document. If fyn is NULL then this operation will delete the target node.

pantoniou commented 1 month ago

Well, duh...

Can you create a PR for this patch?

hq6 commented 1 month ago

https://github.com/pantoniou/libfyaml/pull/117

There appears to be a failing test when I run make check locally, but I'm not sure how your test suite works, and it appears to be failing at the tip of master too, so I don't think it is caused by this PR.

FAIL: testsuite-json.test 256 UGM3 - Spec Example 2.27. Invoice (JSON)
pantoniou commented 1 month ago

What does the test run log contain? Paste it here.

hq6 commented 1 month ago

I'm just running make check.

Could you provide steps for running a specific test on the command line and getting the log?

Also, as I mentioned before, that test is also failing for me on master, so it seems unlikely to be related to this change.

Up to you, but I'm thinking it's not worth blocking this change on fixing the test, unless you've observed this change breaking tests that are working on master.

On Wed, Jun 26, 2024, 12:37 AM Pantelis Antoniou @.***> wrote:

What does the test run log contain? Paste it here.

— Reply to this email directly, view it on GitHub https://github.com/pantoniou/libfyaml/issues/115#issuecomment-2191024519, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAEG5ICECDVPZPYVALM7HHTZJJVUFAVCNFSM6AAAAABJVFQVZCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCOJRGAZDINJRHE . You are receiving this because you authored the thread.Message ID: @.***>

hq6 commented 1 month ago

Actually it looks like this test failure has been happening for a while on master, based on this issue which reports the same issue.