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

Add server-auxiliary-resources-management server-link-auxiliary-type #372

Merged
merged 4 commits into from
Jan 24, 2022

Conversation

csarven
Copy link
Member

@csarven csarven commented Jan 6, 2022

This PR acknowledges the clarification needed in #371 .

This PR describes the server requirement to advertise auxiliary types defined by the Solid Protocol. That includes #auxiliary-resources-web-access-control and #auxiliary-resources-description-resource . The respective link relations with specific rel parameter value are defined in other documents.

@csarven csarven force-pushed the fix/server-link-auxiliary-type branch from 60bc26b to 2d13ea4 Compare January 6, 2022 16:09
@csarven csarven changed the title Add server-link-auxiliary-type Add server-auxiliary-resources-management server-link-auxiliary-type Jan 6, 2022
Copy link
Member

@dmitrizagidulin dmitrizagidulin left a comment

Choose a reason for hiding this comment

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

👍
lgtm.

kjetilk
kjetilk previously requested changes Jan 7, 2022
Copy link
Member

@kjetilk kjetilk left a comment

Choose a reason for hiding this comment

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

I don't feel this is a clarification over #371 , to the contrary, actually.

I think it is important that for now at least, each type should be defined with their own requirement.

In concrete terms, that means that the sentence to require the ACL aux type sits in the WAC spec, whereas the describedby requirement is defined in this spec, but in its section, as suggested in #371 . I suggest we revert the last commit in this PR and then merge both #372 and #371.

Looking ahead, there will most certainly be aux types that will not be supported by all servers, like audit logs, and so a general statement like in L686 seems rather convoluted to me. To write tests for that statement, you'd have to read a lot between the lines, and a test failure will be very confusing to implementers. Also, the protocol spec tests would need to have tests that really belong with the WAC tests.

If we just said L704 and #371, there will be a very clear test, the test will look for the link relation, GET that resource and parse it to determine it is valid RDF. Subsequent tests will then verify the write operations on it. This is simpler, easy to understand and will actually provide guidance for implementers.

With the current text, you'd have to do the same, but for every aux type, and we'd need to have conditions around not only around WAC, but also aux resources. I see absolutely no reason to accept that kind of complexity when the alternative is so much simpler.

@csarven
Copy link
Member Author

csarven commented Jan 7, 2022

I think it is important that for now at least, each type should be defined with their own requirement.

If we are going to use language like "support" (required), it should be consistent. That was part of the point in this PR ( #server-auxiliary-resources-management ) because PR 371 only covered Description Resources. Need the same for WAC.

I'm okay to have either #server-auxiliary-resources-management or
both #server-web-access-control-management and #server-description-resource-management .


Looking ahead, there will most certainly be aux types that will not be supported by all servers, like audit logs,

If listed in the Solid Protocol, it'll be required by all servers.

and so a general statement like in L686 seems rather convoluted to me.

It is not a general statement; literally refers to "auxiliary resources defined by this specification" (which are listed in the same section). It is a known and countable list of things.

If/when an extension system for new auxiliary resources is defined (as I've proposed in #270 (comment) and elsewhere) , the requirement #server-auxiliary-resources-management doesn't apply simply because those are not defined in the Solid Protocol.

To write tests for that statement, you'd have to read a lot between the lines, and a test failure will be very confusing to implementers.

I can understand why splitting the requirement to what can be individually tested is preferable. But the Protocol includes many requirements which in fact includes multiple things that can be testable e.g., a requirement with multiple HTTP methods. And in the tests, they are actually split into separate tests. So I don't see how that's above and beyond different than #server-auxiliary-resources-management .

@kjetilk
Copy link
Member

kjetilk commented Jan 10, 2022

I think it is important that for now at least, each type should be defined with their own requirement.

If we are going to use language like "support" (required), it should be consistent. That was part of the point in this PR ( #server-auxiliary-resources-management ) because PR 371 only covered Description Resources. Need the same for WAC.

Yes, but that belongs in the WAC spec, not here.

I think this is quite important, because we get into a mess both in prose and in tests if we have too many unnecessary instances of requirements that has to be replicated in several specs or , it is a warning sign that we're not doing good job with orthogonality.

I'm okay to have either #server-auxiliary-resources-management or both #server-web-access-control-management and #server-description-resource-management .

I have to admit that I haven't fully internalized our labels yet ;-) But I think I'm not OK with that. This specification should reference WAC to bring along its requirements, it should not require things from there itself. That makes it hard to test and make something that that should be orthogonal like 86.7 degrees or something :-)

It is not a general statement; literally refers to "auxiliary resources defined by this specification" (which are listed in the same section). It is a known and countable list of things.

Sure, and right now, it doesn't matter that much since WAC is actually required, so it doesn't actually matter for implementers, but I think it is a bad pattern that we should avoid already.

I can understand why splitting the requirement to what can be individually tested is preferable. But the Protocol includes many requirements which in fact includes multiple things that can be testable e.g., a requirement with multiple HTTP methods. And in the tests, they are actually split into separate tests. So I don't see how that's above and beyond different than #server-auxiliary-resources-management .

There are certainly a lot of stuff in the spec that is currently hard to test, but that's not a reason to willfully make it hard to test! I think we need to clean up the spec urgently to make it testable, and that will entail reformulating things in the spec, so we shouldn't make this another thing that must be cleaned urgently.

@csarven
Copy link
Member Author

csarven commented Jan 10, 2022

I added #server-link-auxiliary-type which covers both description resource and acl resource, and you seem to be okay with that. But you're not okay with #server-auxiliary-resources-management which happens to take the same approach covering both types.

?

Don't assume that WAC has full knowledge of Solid Protocol or the lifecycle ACL resources associated with resources is same as or derived from Solid Protocol's auxiliary resources. It doesn't. (This is generally talked about in #http-interactions ). Let me give an example: when a resource is deleted, WAC doesn't add rules about deleting the ACL resource. It however notes that obvious practice/possibility #reinstated-resource-permissions :

Implementations might perform cleanup tasks such as deleting the ACL resource that is associated with a resource when the resource is deleted.

It leaves it up to the Protocol that's used with WAC to make that call.

And there are other examples if I'm not mistaken.

So, can't just say "do WAC" and assume that WAC is covering all the relevant aspects of auxiliary resources as far as the Solid Protocol is concerned.

@kjetilk
Copy link
Member

kjetilk commented Jan 11, 2022

I added #server-link-auxiliary-type which covers both description resource and acl resource, and you seem to be okay with that.

Yes, because that defines very concrete behavior that all aux resources as per today will need to do.

But you're not okay with #server-auxiliary-resources-management which happens to take the same approach covering both types.

Yes, because that defines very nebulous and hardly testable behavior, which will certainly not live on into the future.

?

Don't assume that WAC has full knowledge of Solid Protocol or the lifecycle ACL resources associated with resources is same as or derived from Solid Protocol's auxiliary resources. It doesn't.

Of course.

So, can't just say "do WAC" and assume that WAC is covering all the relevant aspects of auxiliary resources as far as the Solid Protocol is concerned.

Certainly. But we can define clear behavior that all covers all types, and then also make it clear what the type specs must define themselves.

Perhaps, it is just that I have a much too clear idea of how I would write this code and these tests. The spec here seems unhelpful, but it might be that I'm not being open minded enough.

I certainly don't think this is worth spending more time on if others are fine with how this is formulated, so let me clear my objection to a neutral.

@kjetilk kjetilk self-requested a review January 11, 2022 00:37
@kjetilk kjetilk dismissed their stale review January 11, 2022 00:38

Don't want to spend more time on this, it will have to be cleaned up anyway.

Copy link
Contributor

@RubenVerborgh RubenVerborgh left a comment

Choose a reason for hiding this comment

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

Without going into the detailed discussion, this is clear to me, excluding the rel comment.

ED/protocol.html Outdated Show resolved Hide resolved
ED/protocol.html Outdated Show resolved Hide resolved
ED/protocol.html Outdated Show resolved Hide resolved
@RubenVerborgh
Copy link
Contributor

@csarven

I'm okay to have either #server-auxiliary-resources-management or
both #server-web-access-control-management and #server-description-resource-management .

I'm okay with both; I don't see #372 as replacing #371; to me, #371 defines a specific type, and #372 a generic mechanism. We could start from #372 and have #371 as one case.

@kjetilk
Copy link
Member

kjetilk commented Jan 18, 2022

@csarven

I'm okay to have either #server-auxiliary-resources-management or
both #server-web-access-control-management and #server-description-resource-management .

I'm okay with both; I don't see #372 as replacing #371; to me, #371 defines a specific type, and #372 a generic mechanism. We could start from #372 and have #371 as one case.

Alright. I think this will be need to be improved in a work to establish hypermedia extension mechanisms around it anyway. Can we agree to merge both, and then we'll see as we write tests?

@csarven
Copy link
Member Author

csarven commented Jan 18, 2022

I'm not okay to merge both PRs. The "both" that I, and I believe, Ruben is referring to is about mentioning the specific support in each ("both") section.

I'm okay to modify this PR to do that, but if we do that, I suggest mentioning the rel value in each of those requirements i.e., the upcoming #server-web-access-control-management and #server-description-resource-management .

However, if we stick to the current PR, hitting merge right now would be sufficient.

@kjetilk
Copy link
Member

kjetilk commented Jan 18, 2022

OK. I have believed for a long time that text, test and implementation should walk hand in hand, as they all contribute to an understanding of good wording of the spec. I don't think either should be subordinated the others, so lets then leave this problem open until we have good tests around them.

ED/protocol.html Outdated
@@ -683,7 +683,7 @@ <h3 property="schema:name">Auxiliary Resources</h3>
<div datatype="rdf:HTML" property="schema:description">
<p>Solid has the notion of <em>auxiliary resources</em> to provide supplementary information such as descriptive metadata, authorization conditions, data shape constraints, digital rights or provenance record about a given resource (hereafter referred as the <em>subject resource</em>), and affects how resources and others associated with it are processed, served or interpreted.</p>

<p id="auxiliary-resources-management">Server manages the association between a subject resource and auxiliary resources defined by this specification. The lifecycle of auxiliary resources defined by this specification depend on the lifecycle of the subject resource that they are associated with.</p>
<p><span about="" id="server-auxiliary-resources-management" rel="spec:requirement" resource="#server-auxiliary-resources-management"><span property="spec:statement"><span rel="spec:requirementSubject" resource="spec:Server">Servers</span> <span rel="spec:requirementLevel" resource="spec:MUST">MUST</span> support auxiliary resources defined by this specification and manage the association between a subject resource and auxiliary resources. The lifecycle of auxiliary resources defined by this specification depends on the lifecycle of the subject resource that they are associated with.</span></span></p>
Copy link
Contributor

Choose a reason for hiding this comment

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

After discussing this with @kjetil, I would just comment that the statement below does seem redundant:

The lifecycle of auxiliary resources defined by this specification depends on the lifecycle of the subject resource that they are associated with.

When it says lifecycle I believe the only relevant part of the lifecycle is actually DELETE and that requirement is already covered (for all auxiliary resources, not just those defined in the spec) by #server-delete-remove-auxilary-resource. If this is a non-normative comment to help understanding then it doesn't need to be part of the requirement statement where it could be perceived as adding to or amending the requirement. Perhaps it could be moved, and simplified to say something like:

When a subject resource is deleted its auxiliary resources are also deleted by the server [#server-delete-remove-auxilary-resource]

Copy link
Member Author

@csarven csarven Jan 24, 2022

Choose a reason for hiding this comment

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

@edwardsph , thanks. I agree (as discussed elsewhere) and updated the text as per your suggestion.

@kjetilk @RubenVerborgh Are we okay to merge this and close 371? This shouldn't drag out longer.

371 and 372 should stick to clarifying the current text / what was in our minds as opposed to introducing anything new or major changes (on auxiliary resources) - those can be in a different PR.

Copy link
Member

@kjetilk kjetilk left a comment

Choose a reason for hiding this comment

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

I still think this doesn't make sense as a requirement... I could live with it if we remove the markup that makes it a requirement.

@csarven
Copy link
Member Author

csarven commented Jan 24, 2022

Updated. It makes the requirement strictly about "support" as per your suggestion. I disagree with it being fluffy or whatever. It is a description of whatever is relevant for management/support.

Copy link
Contributor

@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.

That works better for me now with the requirement reduced to the single sentence.

@csarven csarven deleted the fix/server-link-auxiliary-type branch May 12, 2022 16:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants