Split configuration options by feature - #732
Conversation
c960487 to
661e20b
Compare
Woah!
Anything you can share?
Excellent plan. |
| * of a {@link TargetExtractor}, {@link PreferenceParser}, {@link MetadataParser}, and {@link BodyParser}. | ||
| */ | ||
| export class BasicRequestParser extends RequestParser { | ||
| private readonly targetExtractor!: TargetExtractor; |
There was a problem hiding this comment.
TargetParser then too, I suppose?
There was a problem hiding this comment.
While this was more a result of removing the class MetadataExtractor and using its input classes directly (which were MedataParsers), that's not a bad idea.
Can you explain this one a bit more? I'm sure there was a reason for not passing the store around.
That worries me a bit. The general principle is good; just wonder if we need this in every patcher, or on a higher level. |
I first tried making a nice diagram but it cost me too much time to make it look useful so I just wrote it as nested blocks in a txt file, so it's not something I would put in the documentation 😅 : https://gist.github.com/joachimvh/c02534481045307e398bd044c4923fe0
The main reason I think we had a different store for the patch handler was because it generated internal/quads so we had an extra conversion store in there that always convert to The main reason I want to do this is because it makes our store stack a lot nice and easier to configure. Now you can just configure it like store A -> store B -> etc. Before you would have to branch of when you reached the patch handler since that one needed its own store.
Potentially. It's hard to say until we have a second patcher. Perhaps some of these features could then be moved to a superclass. |
I take it I do like the change, but what I dislike is the conversion logic being part of the patcher. It seems to me that there can be a whole class of patchers that:
Can we perhaps just move 1/2/4/5 to a base class? In a different PR is okay. |
|
I think the Perhaps:
or just even drop the Do we still need thee |
RubenVerborgh
left a comment
There was a problem hiding this comment.
Really excellent work! Much simpler than I thought it could be. Great!
I did not go through everything in detail, since I can just add suggestions in new PRs. Some suggestions already below.
In the comments, try to give a broader context, and describe from the noun rather than "this configuration". I have added some examples.
- In the
authenticationfolder:- Let's name them
debug-test-agent,debug-auth-header,dpop-bearer.
- Let's name them
authorizationacl.json=>webacl.json
identity-email=>identity/email?- Seems all middleware is just header setup? Should that be named headers?
header.json=>constant-headers.jsonwebsocket.json=>updates-via.json
I wonder if we want to add a higher-level grouping above. Such as init, http, idp, ldp, and then have a lot in the latter two folders?
| "@context": "https://linkedsoftwaredependencies.org/bundles/npm/@solid/community-server/^0.0.0/components/context.jsonld", | ||
| "@graph": [ | ||
| { | ||
| "comment": "This extractor always sets the credentials to the fixed value.", |
There was a problem hiding this comment.
| "comment": "This extractor always sets the credentials to the fixed value.", | |
| "warning": "DO NOT USE IN PRODUCTION, ONLY FOR DEVELOPMENT, TESTING, OR DEBUGGING." | |
| "comment": "This extractor always sets the credentials to the fixed value.", |
It not allowed, then we can prepend this to the comment.
Should be added to all classes in this situation.
There was a problem hiding this comment.
It's not allowed since comment is a special predicate known by components.js. Would actually be interesting to have components.js support warning and have it emit warnings when such a file gets imported.
I would not hide the templates inbetween the presets/components/subsystems as that would cause them to be harder to find, and they have a specific purpose. Probably should also rename them so they don't get confused with the storage component. Originally I wanted to put them in the templates folder actually, but that would require us to have another import path in the package.json so components.js could find them. Could still go that route.
I don't think those were ever "needed", it just was the current way to name the config options.
My problem was that you can't have line breaks in json strings, so I preferred to keep the descriptions short. More extensive descriptions could go into the accompanying markdown file. Regarding that markdown file, I'm thinking now that it might be better to just have a
Could do that. The CORS handler sort of breaks that though, since it will resolve OPTIONS requests itself instead of passing them on to the http handlers.
I was also thinking about something like that when making this. The one issue is consistency. I really like how in the current solution you just have to pick something from every folder. So if we did a higher-level grouping, everything would need to be 2 levels deep to be consistent. The main issue there is that several features are sort of independent, so there would be multiple high-level folders with only 1 entry (unless we make some very generic ones). |
|
Okay with all of the above!
That is great indeed (and we should emphasize this more).
Not necessarily though. We can keep the "pick one thing from every (leaf) folder", even if some are 1 level and others are 2. The current breadth might be a bit intimidating. |
I agree it might be a bit intimidating, and it is quite certainly going to grow more in the future, so finding a good solution for this would be nice. Always going to leaf folders is not going to work with the current solution though. Currently leaf folders are used for specific parts the user should not choose from. E.g., We could phrase it as "find the first folder that contains 1 or more config files and choose something there". This would work since those high level folders are not going to contain config files. I do feel like this woul drastically complicate the understanding of how those folders are supposed to be used though. |
I think that one's okay. Pick something from every first level with files. If needed, we can |
This way the chain of ResourceStores is a bit easier to configure. This commit also updates the SparqlUpdatePatchHandler to keep the metadata and content-type of the resource that is being modified.
This way the MetadataExtractor class is not needed anymore after a small change in the request parser.
|
I had a look into how I could add high-level folders. Below a suggestion. The "from" statements are references to how that folder is currently named.
This way everything is in a high-level folder instead of the separation suggested before but I think something like this looks fine? Other possibilities include having a
I kept |
|
Excellent, looks good to me! |
|
Last (?) changes: config templates are now in the templates folder, configs are split over high-level folders as discussed above, several configs have been renamed as suggested and also to have some more consistency. One other big change is that I moved the documentation to the config folder. There is now a readme in the config folder and in each of the high-level folders. This way the config documentation can be seen by going to config folder on github, and the documentation of the options can be seen by going into the high-level folders. The documentation could be more extensive, both in the markdown files as in the configs, but that would still cost quite a bit more time so I'm going to leave that as it currently is. This should already be a good starting point for people interested in the configs. |
Would advise not just going through the git diff for this.
Related to #261. Closes #295 (imo).
I have completely refactored how the configuration presets are stored in an attempt to make it more clear what is happening and make it easier to create new configurations, based on my experiences and what I heard from others using CSS.
I started out by creating a diagram of how everything is connected in our current architecture (turns out it's already pretty big) and marking which parts should be easy to change. This corresponds to a list of (mostly) orthogonal features that I would want to easily enable/disable or modify when creating a new config.
How I then implemented this is by making a folder for every feature. In each of those folders there are then multiple config that set the values that are specifically needed for that feature. E.g., in the
authorizationfolder there is anacl.jsonthat uses acl authorization and anallow-everything.jsonthat disables authorization.The idea is then that if a new user wants to set up CSS with specific features they just have to import 1 file from every folder and they have a completely valid CSS config with all the specifics they want. It's also set up in such a way that it is easy to add values in certain cases (e.g. if you just want to add a new converter to the conversion stack).
I added a a
Configuration-format.mdwith some more documentation on how it all works. This is also why I recommend not (just) looking at this in the git diff, looking at it in a file explorer gives a better overview.Some minor changes were also made to streamline certain parts of the config:
PatchHandlersnow take a target store in thehandlefunction. This makes it so much easier to just chain stores together. I also used this opportunity to clean up some things in our patch handler (it now tries to convert back to the original content type and keeps metadata).MetadataExtractoris replaced by aParallelHandler. Makes it more consistent with theMetadataWriter.This is mostly finished, there are still a few things but this way a discussion can already be started since this is a subjective solution of course.
TODO: