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

PUT or PATCH on auxiliary resources #65

Draft
wants to merge 51 commits into
base: main
Choose a base branch
from

Conversation

kjetilk
Copy link
Collaborator

@kjetilk kjetilk commented Dec 28, 2021

The following test looks correct to me, but all three servers fail, so I guess not...

CSS and ESS do not appear to support description resources (and they not appear to be required by the spec, which may be an omission).

NSS fails with the following:

2021-12-28 16:04:23,216 DEBUG [com.int.karate] (main) request:
1 > PUT https://solid-test-suite-alice.inrupt.net/shared-test/qVerO9/nWRqOd/LODAWZ/.meta
1 > Authorization: DPoP ***nDlAmQ
1 > User-Agent: Solid-Conformance-Test-Suite
1 > DPoP: ***X74IPQ
1 > Content-Type: text/turtle; charset=UTF-8
1 > Content-Length: 21
1 > Host: solid-test-suite-alice.inrupt.net
1 > Connection: Keep-Alive
1 > Accept-Encoding: gzip,deflate


2021-12-28 16:04:23,657 DEBUG [com.int.karate] (main) response time in milliseconds: 426
1 < 415
1 < X-Powered-By: solid-server/5.6.16
1 < Vary: Accept, Authorization, Origin
1 < Access-Control-Allow-Credentials: true
1 < Access-Control-Expose-Headers: Authorization, User, Location, Link, Vary, Last-Modified, ETag, Accept-Patch, Accept-Post, Updates-Via, Allow, WAC-Allow, Content-Length, WWW-Authenticate, MS-Author-Via, X-Powered-By
1 < Allow: OPTIONS, HEAD, GET, PATCH, POST, PUT, DELETE
1 < Link: <.meta.acl>; rel="acl", <.meta.meta>; rel="describedBy", <http://www.w3.org/ns/ldp#Resource>; rel="type"
1 < MS-Author-Via: SPARQL
1 < Content-Type: text/plain; charset=utf-8
1 < Content-Length: 33
1 < ETag: W/"21-e18QFV2ocTdTJSsOtDgYux+uYSE"
1 < Date: Tue, 28 Dec 2021 16:04:23 GMT
1 < Connection: keep-alive
1 < Set-Cookie: nssidp.sid=s%3APUX5s0m8UymjXV3ou01X4NNwPuuRcQzS.pK8zFQnl8ok%2BS%2BTifyoKob6M5Oxals%2FnHptCgjBmrGA; Max-Age=1640793863000; Expires=Mon, 05 Sep 54016 09:07:43 GMT; Domain=inrupt.net; Secure
RDF file contains invalid syntax

it may looks like the request body of the PUT request is not getting sent to the server, and I don't understand why. So, I push it so that others can have a look.

Once this works, more tests for resources and also PATCH can be added.

@kjetilk kjetilk marked this pull request as draft December 28, 2021 16:11
Copy link
Collaborator

@csarven csarven left a comment

Choose a reason for hiding this comment

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

Test looks right but I can't read Karate :)

Is the representation body (the string) goes out as is or any transformation in place eg., baseURL being determined... and the body includes @base or the URIs in the statements are updated.

The request that's going out is:

Content-Type: text/turtle; charset=UTF-8

but the Scenario here does not include charset. It seems to get added. Might want to look into this.

May also want to look into whether the servers can handle the charset properly.

@csarven
Copy link
Collaborator

csarven commented Dec 29, 2021

Copy link
Collaborator

@edwardsph edwardsph left a comment

Choose a reason for hiding this comment

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

Is PATCH still to be added?
[EDIT] I missed the other comments from above so ignore this question

protocol/solid-protocol-test-manifest.ttl Outdated Show resolved Hide resolved
Comment on lines +10 to +11
* def response = clients.alice.sendAuthorized('GET', container.url, null, null)
* def links = parseLinkHeaders(response.headers)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
* def response = clients.alice.sendAuthorized('GET', container.url, null, null)
* def links = parseLinkHeaders(response.headers)
Given url container.url
And headers clients.alice.getAuthHeaders('GET', container.url)
When method GET
* def links = parseLinkHeaders(responseHeaders)

Why not use the normal GET? I know it is longer but I see no need for the special case of sendAuthorized()

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I did that because I don't think of that as a test in its own right, that's just there to get the link. No strong opinions though.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That makes sense to me and I have done similar elsewhere (e.g. every time we create a test container or resource). In this case, I wonder if we should provide overrides to the sendAuthorized method so that there are simpler variants making the test easier to read. Let's discuss.

@edwardsph
Copy link
Collaborator

Did the change to the RDF being sent fix this? The reason the body appeared not to be sent is that Karate doesn't interpret it correctly and just doesn't show it. We have an issue open to improve this. The content length was 21 indicating it was sent.
The charset is added if you don't specify one - we can turn this off if needed.

kjetilk and others added 4 commits January 7, 2022 22:20
Co-authored-by: Pete Edwards <edwardsph@users.noreply.github.com>
Co-authored-by: Pete Edwards <edwardsph@users.noreply.github.com>
Co-authored-by: Pete Edwards <edwardsph@users.noreply.github.com>
Co-authored-by: Pete Edwards <edwardsph@users.noreply.github.com>
@kjetilk
Copy link
Collaborator Author

kjetilk commented Jan 7, 2022

It appears that it is an unrelated bug in NSS that causes the NSS test failure, and I agree, the Content-Length hints that it was sent. So, I suppose we can say that this test is actually valid, then? We need to resolve solid/specification#371 and/or solid/specification#372 and then this would be OK (but needs more tests).

@sonarcloud
Copy link

sonarcloud bot commented Sep 22, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

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