comunica / comunica-feature-solid

📬 Comunica packages for query execution over Solid data pods
Other
3 stars 4 forks source link

INSERT DATA query on solid pod crashes in browser - again #30

Closed remodietlicher closed 2 years ago

remodietlicher commented 2 years ago

This issue has been modified by @rubensworks to describe the intended solution

Problem:

The external solid-client-authn-js has a bug that makes it pass incorrect content types on PUT requests.

This only occurs in browsers, not in Node.js.

Possible solution:

Since the bug only seems to occur when using headers using a Headers object, and not when passing headers as a plain record, we may be able to work around it by just converting our Headers object to a record.

Implementation suggestions:

The same workaround as here could be applied here.


Original issue:

Issue type:


Description:

I'm trying to do an insert query using @comunica/query-sparql-solid in a next.js + typescript app. I recently ran into another problem reported here (https://github.com/comunica/comunica-feature-solid/issues/22) with a very similar setup. I since switched from the deprecated @comunica/actor-init-sparql-solid to @comunica/query-sparql-solid. Therefore, the same pre-condition applies: the same query works when executed via command line. This is what I'm trying to do in my TypeScript code:

    const insertQuery = `
      INSERT DATA {
        <${
          getDefaultSession().info.webId?.split("#")[0]
        }#Seppli> <http://xmlns.com/foaf/0.1/name> 'Seppli'.
      }
    `;

    console.log(insertQuery);

    await engine.queryVoid(insertQuery, {
      sources: [getDefaultSession().info.webId!],
      "@comunica/actor-http-inrupt-solid-client-authn:session":
        getDefaultSession(),
    });

Full code available here. But it fails with this error: Unsupported patch content type: text/plain.


Environment:

comunica-solid-react-test (main)$ ./node_modules/.bin/comunica-sparql-solid -v software version
Comunica Engine 2.0.6
node v16.14.0
npm 8.3.1
yarn 1.22.17
Operating System linux (Linux 5.13.0-37-generic)

Crash log:

https://pastebin.com/TMexGNpM


Bounty

A bounty has been placed on this issue by:

Comunica Association
€272

Click here to learn more if you're interested in claiming this bounty by resolving this issue.

github-actions[bot] commented 2 years ago

Thanks for reporting!

rubensworks commented 2 years ago

This seems to be unrelated to #22 though.

Your server produces this error:

Error: Could not update https://remo.solid.tschenten.ch/profile/card (HTTP status 415): Unsupported patch content type: text/plain

So it seems like your server is not accepting the media type using which comunica is sending a patch.

What server software are you using?

Could you also provide the full request info that is being sent to the server? (can be obtained by executing the query on the command-line with -l debug)

remodietlicher commented 2 years ago

Indeed, sorry if relating to #22 caused confusion. I wanted to mention it because it's a very similar setup.

I'm using Node Solid Server 5.6.16. I just tried with https://solidcommunity.net/ (Node Solid Server 5.7.1) aswell and get the same error: "Unsupported patch content type: text/plain"

rubensworks commented 2 years ago

@remodietlicher This seems to be a (new) problem in the Node Solid Server implementation (which I assume you are using), since everything works fine via instances of Community Solid Server and Enterprise Solid Server.

Could you open an issue for it on the Node Solid Server issue tracker? We can then add it to the list of known issues when interacting with NSS: https://github.com/comunica/comunica-feature-solid/tree/master/engines/query-sparql-solid#known-issues

remodietlicher commented 2 years ago

Can you elaborate how you got this to work with ESS? When I change to oidcIssuer: "https://broker.pod.inrupt.com" in my test code, I get a very similar error: 415 - "Unsupported Media Type".

rubensworks commented 2 years ago

There are some known issues still with ESS (which have been reported a while ago to Inrupt): https://github.com/comunica/comunica-feature-solid/tree/master/engines/query-sparql-solid#known-issues

From my experience, CSS seems to be the most spec-compliant in this regard, so I would recommend using that one for testing until the issues in NSS and ESS have been fixed.

remodietlicher commented 2 years ago

Sorry for being tenacious about this and thanks again for the suggestion regarding CSS. I now did just (solidweb.me which looks like it has the latest CSS installed) that and get something along the lines of unsupported media type again (https://pastebin.com/3kPKmPTs). What confuses me is that executing the same query on the command line works:

./node_modules/.bin/comunica-sparql-solid --idp https://solidweb.me https://solidweb.me/RemoDietlicher/profile/card "INSERT DATA {
        <https://solidweb.me/RemoDietlicher/profile/card#Heidi> <http://xmlns.com/foaf/0.1/name> 'Heidi'
      }"

Produces

<#Heidi> <http://xmlns.com/foaf/0.1/name> "Heidi".

in https://solidweb.me/RemoDietlicher/profile/card. Is there something wrong with my code? https://github.com/remodietlicher/comunica-solid-react-test/blob/main/pages/index.tsx

rubensworks commented 2 years ago

Your code looks correct to me, so not sure what is going wrong here.

In order to rule out package version issues, could you try removing your node_modules and lockfile, and reinstalling?

remodietlicher commented 2 years ago

rm -rf node_modules yarn.lock && yarn install && yarn dev didn't change anything...

rubensworks commented 2 years ago

Thanks for checking. Something must still be going wrong then, so I'm reopening this issue.

Still not sure what is going on though (especially since it works on the CLI), because we have similar code working fine on our end.

remodietlicher commented 2 years ago

Can you share a minimal working version of your code? I'm happy to hunt for some minor differences that might be causing the problem.

rubensworks commented 2 years ago

I don't have anything easily shareable lying around myself, but I'll ask some other people.

remodietlicher commented 2 years ago

I just figured out what the issue was! I used the sparql source type for the update query instead of putLdp. Good thing I listened to your talk last Tuesday :)

Now it works for solidcommunity.net (NSS), solidweb.me (CSS) as well as broker.pod.inrupt.com (ESS) :tada:

rubensworks commented 2 years ago

Ah, that explains it. Glad to hear it's solved! :-)

remodietlicher commented 2 years ago

I got excited a bit too early. putLdp works, but I didn't notice that it replaces the entire content of the destination with the content of my query. I guess what I would need is patchSparqlUpdate to patch the destination. Unfortunately, this still does not produce the right header but gives 'content type': 'text/plain' which the server then complains about. I'm lost here because on the command line it works and an insert query actually produces 'content type': 'application/sparql-update' as verified by running ./node_modules/.bin/comunica-sparql-solid with -l debug. What would be really great is having this type of documentation for a JavaScript application aswell!

remodietlicher commented 2 years ago

Maybe this is worth noting: I played around a bit using N3.Store as destination instead of the solid servers and noticed that I get the same error as reported above when using the QueryEngine from @comunica/query-sparql-solid but it works fine if I use QueryEngine from @comunica/query-sparql. Maybe something is wrong with my (the default?) setup of query-sparql-solid?

rubensworks commented 2 years ago

I get the same error as reported above when using the QueryEngine from @comunica/query-sparql-solid but it works fine if I use QueryEngine from @comunica/query-sparql

That's interesting. That means that either you're using an older version of @comunica/query-sparql as a dependency of @comunica/query-sparql-solid (which you can fix by resetting your lockfile, removing node_modules, and reinstalling), or there's a bug in Inrupt's solid-client-authn-js libraries.

Before we dive into the second, can you try the lockfile reset?

What would be really great is having this type of documentation for a JavaScript application aswell!

Like this? https://github.com/comunica/comunica-feature-solid/tree/master/engines/query-sparql-solid#usage-within-application

remodietlicher commented 2 years ago

I don't have access to the code right now so I can only try in the evening. I doubt that it will help as I set up this project from scratch just this weekend. Wouldn't the command line struggle aswell if the culprit was Inrupt's libs? I really suspect that I'm just missing something in JavaScript to produce the right (same as via CLI) header...

Regarding documentation: Exactly like this but for all query types. The SELECT query in the README you cite works for me. More something like this: https://comunica.dev/docs/query/getting_started/update_app/ but for solid. Maybe just as a new subsection "Querying with Authentication" or something like that?

rubensworks commented 2 years ago

I really suspect that I'm just missing something in JavaScript to produce the right (same as via CLI) header...

You mentioned forcing putLdp as destination type. Have you already tried removing any forced destination types completely? (which is what the CLI does by default).

Regarding documentation: Exactly like this but for all query types. The SELECT query in the README you cite works for me. More something like this: https://comunica.dev/docs/query/getting_started/update_app/ but for solid. Maybe just as a new subsection "Querying with Authentication" or something like that?

Sure! Feel free to make an issue and/or pull request for this.

rubensworks commented 2 years ago

@remodietlicher I just had another look, and I'm unable to reproduce your problem (tested on a local Solid Community Server instance). The following code works fine for me:

  const session = await interactiveLogin({
        oidcIssuer: 'http://localhost:3000/',
    });

    // Create test.ttl (did not exist before)
    await myEngine.queryVoid(`INSERT DATA { <ex:s> <ex:p> <ex:o1> }`, {
        sources: [ 'http://localhost:3000/test.ttl' ],
        '@comunica/actor-http-inrupt-solid-client-authn:session': session,
    });

    // Modify existing test.ttl
    await myEngine.queryVoid(`INSERT DATA { <ex:s> <ex:p> <ex:o2> }`, {
        sources: [ 'http://localhost:3000/test.ttl' ],
        '@comunica/actor-http-inrupt-solid-client-authn:session': session,
    });

    await session.logout();
remodietlicher commented 2 years ago

Not providing a destination type doesn't help...

Thanks a lot for the snippet! I made this one run myself against https://solidcommunity.net. Unless I missed something, this is how you would run it in a node program, right? When running the exact same query in the browser, I still get the Content-Type: text/plain HTML header... Note: when changing the engine to @comunica/query-sparql the header is correct (but of course auth doesn't work).

I'm very happy to open an MR for the documentation once I know what it takes to make the query work in the browser.

rubensworks commented 2 years ago

When running the exact same query in the browser, I still get the Content-Type: text/plain HTML header...

Ah ok, I forgot about that part, was only checking in Node.

Note: when changing the engine to @comunica/query-sparql the header is correct (but of course auth doesn't work).

Large chance that it's a problem with @inrupt/solid-client-authn-browser then, since our code is the same on browser and Node.

You can confirm this by inspecting the request that Comunica does when it does a PUT/PATCH request to the server, and copy the body + content-type header. If you perform exactly the same request manually using @inrupt/solid-client-authn-browser's authenticated fetch, and it gives you the same problem, then we know it's a problem on their end, and we can report an issue there.

remodietlicher commented 2 years ago

I didn't really get how to construct the right request with @inrupt/solid-client-authn-browser so I used the @inrupt/solid-client library (saveSolidDatasetAt) which looks like it uses fetch under the hood.

At least when using @inrupt/solid-client, it looks to me like the Inrupt libs are doing the right thing. If solid-client can do it, shouldn't comunica be able to construct the same requests? I have a very weak grasp on what's going on behind the scenes here, so it might well be that I miss something. In that case, could you provide some guidance on how to construct a fetch call manually (it's not clear to me how to set the init parameter of fetch)?

Following are the results from testing put and update queries with comunica solid and solid-client.

Comunica solid with "putLdp" type:

Content-Type:

text/plain;charset=UTF-8

Payload:

https://example.org/ex http://www.w3.org/1999/02/22-rdf-syntax-ns#type "2022/5/6 22:14:38" .

Comunica solid with "patchSparqlUpdate" type:

Content-Type:

text/plain;charset=UTF-8

Payload:

INSERT DATA { https://example.org/ex http://www.w3.org/1999/02/22-rdf-syntax-ns#type "2022/5/6 22:16:56" . }

Comunica solid without type:

Content-Type:

text/plain;charset=UTF-8

Payload:

INSERT DATA { https://example.org/ex http://www.w3.org/1999/02/22-rdf-syntax-ns#type "2022/5/6 22:16:56" . }

Inrupt solid-client createSolidDataset (put):

Code

  const getDatasetHelper = async () => {
    const myDataset = await getSolidDataset(
      getDefaultSession().info.webId!.replace("profile/card#me", "test2.ttl"),
      { fetch: fetch }
    );
    console.log(myDataset);
  };

  const saveDatasetHelper = async () => {
    let dummyDataset = createSolidDataset();
    const dummyThing = buildThing(createThing({ name: "ex" }))
      .addUrl(RDF.type, "https://example.org/test")
      .build();

    dummyDataset = setThing(dummyDataset, dummyThing);

    await saveSolidDatasetAt(
      getDefaultSession().info.webId!.replace("profile/card#me", "test2.ttl"),
      dummyDataset,
      { fetch: fetch }
    );

Content-Type:

text/turtle

Payload:

<#ex> a https://example.org/test.

Inrupt solid-client getSolidDataset (patch):

Code

    const now = new Date();
    let myDataset = await getSolidDataset(
      getDefaultSession().info.webId!.replace("profile/card#me", "test2.ttl"),
      { fetch: fetch } // fetch from authenticated session
    );

    let exThing = getThing(
      myDataset,
      getDefaultSession().info.webId!.replace("profile/card#me", "test2.ttl#ex")
    );

    exThing = addStringNoLocale(
      exThing,
      RDF.type,
      `${now.getFullYear()}/${now.getMonth()}/${now.getDay()} ${now.getHours()}:${now.getMinutes()}:${now.getSeconds()}}`
    );

    myDataset = setThing(myDataset, exThing);
    await saveSolidDatasetAt(
      getDefaultSession().info.webId!.replace("profile/card#me", "test2.ttl"),
      myDataset,
      { fetch: fetch }
    );

Content-Type:

application/sparql-update

Payload:

INSERT DATA {https://remo-test-1.solidcommunity.net/test2.ttl#ex a "2022/5/6 22:23:14}".};

rubensworks commented 2 years ago

In that case, could you provide some guidance on how to construct a fetch call manually (it's not clear to me how to set the init parameter of fetch)?

It just follows the standard fetch API. Here's an example on how to use it: https://stackoverflow.com/questions/38156239/how-to-set-the-content-type-of-request-header-when-using-fetch-api

remodietlicher commented 2 years ago

Thanks for making the link to the standard fetch API - I guess I outed myself as a non-JavaScript/web-native ;)

Here is what I tried with fetch from @inrupt/solid-client-authn-browser:

PUT:

    fetch(
      getDefaultSession().info.webId!.replace("profile/card#me", "test3.ttl"),
      {
        method: "PUT",
        headers: {
          "Content-Type": "text/plain;charset=UTF-8",
          // "Content-Type": "text/turtle",
        },
        body: `<ex:s> <ex:p> <ex:o1>.`,
      }
    );

PATCH:

    fetch(
      getDefaultSession().info.webId!.replace("profile/card#me", "test3.ttl"),
      {
        method: "PATCH",
        headers: {
          "Content-Type": "text/plain;charset=UTF-8",
          // "Content-Type": "application/sparql-update",
        },
        body: `INSERT DATA { <ex:s> <ex:p> <ex:o2> }`,
      }
    );

With the way it is now, I get exactly the error above: unsupported media type. When I use the headers that are currently commented out (text/turtle for PUT and application/sparql-update for PATCH) it works. Just for fun I tried PUT with text/plain;charset=UTF-8 and then PATCH with application/sparql-update which gives 500 Internal Server Error.

In my opinion, whoever sets the text/plain header is to blame, is that in @inrupt/solid-client-authn-browser?

rubensworks commented 2 years ago

With the way it is now, I get exactly the error above: unsupported media type. When I use the headers that are currently commented out (text/turtle for PUT and application/sparql-update for PATCH) it works.

The commented-out parts are the things we want, and those are what Comunica should be sending to fetch. So if things work when passing the correct headers directly, then the problem may not be in @inrupt/solid-client-authn-browser after all.

However, there's one last thing we can try, and that's passing a Headers object (which is what Comunica passes) instead of a plain object. Because I remember there were some problems related to that in @inrupt/solid-client-authn-browser.

remodietlicher commented 2 years ago

Good catch! fetch will indeed produce the text/plain headers when passing a Headers object from the standard lib!

I'll open a bug report at https://github.com/inrupt/solid-client-authn-js.

rubensworks commented 2 years ago

Great, thanks for following this up @remodietlicher 🙂

rubensworks commented 2 years ago

@remodietlicher I see there hasn't been a reply yet on the issue you posted at Inrupt's issue tracker. If this is a blocker for you, you're definitely welcome to submit a PR here to comunica with a temporary workaround. There are several similar workarounds for bugs in other places already :-)

We could basically apply the same workaround as here to here.

ludwigschub commented 2 years ago

As discussed with @rubensworks, I will work on this issue via the Comunica Association.