Unfortunately, we didn't manage to have full attendance for our meeting in Boston where we discussed the choice of refactoring this module, vs rewriting it from scratch with a deliberate architecture. It has become a recurrent conversation on gitter, but I feel those conversations have not been very productive due to the constraints of the chat as a medium. Perhaps a github issue is better suited for that conversation. For us developers, I think it is very important we can have a resolution to this.
A useful starting point is the document that @RubenVerborgh produced for the Boston meeting, and that him, I and @justinwb formed consensus around. I'll paste it here since @RubenVerborgh is travelling, and so that we have everything in one place:
Purpose: this document details why the node-solid-server v4/v5 code should be considered end-of-life, and why we should consider a rewrite rather than a refactoring for v6. Furthermore, it details a high-level timeline and approach for such a rewrite.
Status:
2018-08-24: drafted by Ruben Verborgh, open for comments
2018-09-20: Edited and amended by Kjetil Kjernsmo for suitability as a github issue.
The server had no upfront architecture, and the current code has organically grown.
The resulting code structure is very difficult and hard to maintain. Changing components is hard, for instance:
One piece of knowledge/functionality is encoded in multiple places
The mapping between a URL and the filesystem is implemented in multiple places.
The request object is frequently written to.
Changes in one place ripple through everywhere
Assessing the correctness of pull requests is difficult
Globbing is implemented as follows:
It is hard to do integration tests (see for instance https://github.com/solid/node-solid-server/blob/v4.1.3/test/integration/acl-oidc-test.js, which has complex setup and testing code, and is not actually testing what it claims it is, only status codes)
Due to extensive use of global state, like detailed above, the code cannot be unit tested.
Interfaces aren’t clear.
Development time is increased since simple tests cannot be made, and therefore, it is far more likely that bugs are introduced without detection.
Tests appear to have been written to test current code behavior rather than specified behavior.
A test titled "user1 should be able to access test directory when origin is invalid", which should not be the case.
While this issue can be fixed through refactoring, the lack of unit tests makes it difficult to assess the correctness of the refactoring.
Given all of the above, we conclude that:
In order to avoid encountering the same problems of v4/v5 again, we should:
- using different backend storage (Amazon S3, triplestores, hybrid, …, see #702 however),
I think #702 actually makes a very good case for a rewrite. Dmitri is doing hard work, but #702 is nowhere near finished—it doesn't even touch any LDP code yet. Based on the above I would make the case that doing #702 in a stable way will require more effort through a refactor than through a rewrite.
Following on something I wrote in #672 - having separate OIDC provider and separate pure dataset pod or pod that also has OIDC provider feature enabled. If current architecture doesn't allow easy separation of those features I would see it as :+1: for a clean rewrite.
I'd like to add some perspectives:
First, I'd like to add my experience about what is actually hard in development. In the beginning of a project, everything is somewhat nebulous and fluid, and you learn as you go, and you make the changes that seem reasonable at the time. You have all kinds of concerns and pushbacks, and you code to a changing world that you do not fully understand at any point in time. This scrambling phase is a really hard development phase to be in. Thus, my desire to rewrite is not born from being judgmental about the current code, to the contrary, I acknowledge that it was written in a different phase of the project, where they were coding to exactly these complexities. Code written under such conditions will necessarily look bad with the benefit of hindsight. That's OK, it is normal, and being efficient under such conditions is to be admired. It is true that many developers will look at other people's code with a certain disdain, but it shouldn't be taken as an argument against a rewrite, it all depends on what phase of a project the code was written in.
However, once you enter a new phase of the project, one when things aren't changing under your feet, when you have the benefit of hindsight, you aren't dealing with all kinds of concerns and pushbacks, replicating that work should normally not be very difficult.
I would normally be against big rewrites in the first phase, unless the codebase is very small. I have been to a few processes where it has been a bad idea to rewrite stuff in that phase.
I hope that the Solid project has entered that much more stable phase now, on the server side. We should know well how the server should behave now. For example, we now know which OIDC libraries are maintained, that knowledge is a luxury our predecessors didn't have.
Thus, a rewrite is not about passing judgment on the current codebase, it is just that one shouldn't be afraid to throw away code if you are in a phase where lessons already have been learned. In fact, that should be a normal thing to do in a long-duration project.
Obviously, it might be that we have not entered that phase yet, that there are still big and open issues. I suppose, https://github.com/solid/solid/issues/152 may be such an issue.
This possibility should be balanced against other problems with the codebase, however.
As mentioned above, rewrites when requirements are changing is in my experience not viable, it takes too long to get back up to parity, and when you reach parity, you will be in the same situation that you were before the rewrite started and producing code in the same environment, which is really hard.
Refactoring is also viable when there was a rough but not thorough architecture to beginning with. Then, with the benefit of hindsight and the benefit of understanding possible frameworks that the architecture could use, refactoring into a framework is something that I have advocated in the past (in a project that was rewritten, when I advocated a refactoring effort).
I have not been working with NSS for very long, but I can say that it is a difficult codebase to get into, taking into the perspective that the spec is pretty simple, and something I have done much of in other code. It is due to two things, mainly; insufficient test coverage and that knowledge of the whole system is often required to understand small changes.
It is not given that a refactoring effort will take less time to do rather than a rewrite, as on-boarding will be slow, and those coming on board cannot easily draw on the experiences of the predecessors as they are not any longer working full time on the project.
I think it is very important that the server has been written by people who are likely to stay with the project for a long time. It makes little sense to retain code that has been in the difficult scrambling phase if none of those who worked at it at the time is working on it continuously.
Since the spec is fairly simple, it would be surprised if we couldn't make a rewrite much simpler than the current code. This is not about making the complicated second system, to the contrary, it is about writing a simpler and better designed system because we have the benefit of hindsight. A case in point is @timbl's rewrite of https://github.com/solid/solid-permissions into https://github.com/solid/acl-check/ where the number of lines were reduced from ~2000 to ~125. While such a reduction is unlikely, it is the kind of goals we would reasonably have.
It is not at all clear to me how one would go about to refactor the current code. I just don't see how that could be done, and so, I would challenge those that advocate refactoring the system to come forward with a concrete plan on how to do that.
Taking another outlook, it is not unlikely that we will be writing the server again and again, since it is supposed to be pretty simple. It may be required to have the server in several languages and environments. There are already several implementations, and to get e.g. Nextcloud on board would require another PHP server that is suited for their purpose.
It may be substantial business opportunities in adapting servers for enterprise environments, and so, it is important that we have a very good understanding of what it costs to write one. The experience that the previous implementors had is not relevant to this problem, they were programming to a much more dynamic world.
We are not proposing a rewrite to produce a larger system, we are proposing to write a simpler system, but one that benefits from the experiences already gained. It is not given that this will be more expensive than refactoring, given that we do not know how a refactoring would look like. Very substantial changes are required to secure the server, and we also need to make the onboarding process simpler, even though we expect fewer people to work on the server side than the client side.
Following on something I wrote in #672 - having separate OIDC provider and separate pure dataset pod or pod that also has OIDC provider feature enabled. If current architecture doesn't allow easy separation of those features I would see it as +1 for a clean rewrite.
Yeah, I suppose it isn't that hard, but I think it is really important to get WebID-OIDC to work by a trivial addition to any compliant OIDC service.
I just want to add a piece of feedback here: I always wanted to rewrite this library! After a few months when things were changing I did want to rewrite it again, so you have a strong support from me (one of the first authors of the original library).
I think the rewrite will be much faster and much more informed today than it was then! I agree with all the points you are making, they are all very valid and really true.
During free time (back then), I ended up rewriting node solid, layering the different components (I can find it if useful), but unfortunately, I never completed it. The idea was to have an LDP library with different storage support which would provide APIs to read/write to the storage, this was wrapped in an ACL layer that would check permission and by an authentication layer that would check the identity.
The ldp library is a good starting point (and the rdf-store library with different storage support that @bergos and I made would make it a lot easier too.
@nicola thanks for the feedback! Do you have any thoughts regarding the approach
Or a combination of the above?
Thanks a lot, @nicola ! It is good to know that you don't just feel we're trampling on your work! As I said, you did this in a very different environment than we're going to face.
I moved already some code (parsers and serializers) to the RDFJS organization for the RDFJS library/framework. That means I removed the rdf-ext dependency and they work with the reference data model or any other RDFJS data factory. My next steps would be (ordered based on the dependencies between the libraries):
The WebID authentications should be implemented as Passport.js modules as it's the de facto standard for node authentication modules.
The mentioned ldp package uses blob-store to access files.
In general it would be good to check which libraries/frameworks/interfaces/abstraction could be used.
@RubenVerborgh @kjetilk thanks for putting this together, and the wonderful ideas for type script interfaces
@bergos thanks for the pointers! I'm also a big fan or passport.js.
Re : refactoring or rewriting, my personal take is that, first and foremost any and all code contributions to node solid server, are a huge plus, and refactoring tends to pay for itself in the longer term. Comparing the merits, refactoring has some of advantages over rewriting
A full rewrite is also a viable strategy. I see it as slightly more risky. Some rewrites are spectacularly successful. Some never make it. Whereas refactoring, by definition keeps exactly the same functionality but improves the code base for those that come after, normally in a time limited way. I can live with either path.
Some specific comments :
Maintenance fixes are still released as v5.0.x, but no more new features.
This sounds a bit like a code freeze to me. As a user, app dev, and occasional maintainer of solid systems, relevant to me is : how long would there be a feature freeze? My estimate would be a rewrite to take c. one year, and I'd be interested in the timelines of those that might like to work on this.
As soon as the server has feature parity is achieved
What would be the metric for achieving this. We dont have full test coverage on the server side, for example for command line switches (not even sure how to do that!). And barely any test coverage for apps. How will we ensure apps, and also servers, don't break upgrading from the old server to the new? We've seen in the past large companies make breaking changes to systems. That can sometimes become a talking point. Creating invisible upgrade paths helps create good will and maintains the public contract.
That said, it's not my decision to make, as others will be doing the work, but I will follow with great interest! Just my 2 cents!
Just a small remark on
How will we ensure apps, and also servers, don't break upgrading from the old server to the new?
Note that this equally holds for refactoring and rewriting, given the current lack (and impossibly of) many unit tests.
Regarding:
Comparing the merits, refactoring has some of advantages over rewriting
You can tackle it in stages
You can try to ensure regression, feature parity, add tests
This is assuming the code _can_ be refactored, in a way that is not merely an in-place rewrite of everything. My arguments above aim suggest that this is not the case for this particular codebase.
We would need a proposal for concrete stages from someone. My opinion is that the code is too tangled for that.
You can measure progress
Same with a rewrite.
I just found my rewriting of solid is actually already on github and it is here: https://github.com/nicola/node-solid
This uses the LDP library and rdf-store
@nicola Thanks, any assessment on completeness besides what the README says?
Re : refactoring or rewriting, my personal take is that, first and foremost any and all code contributions to node solid server, are a huge plus, and refactoring tends to pay for itself in the longer term. Comparing the merits, refactoring has some of advantages over rewriting
Actually, I think that is an oversimplification. The devil is in the details, and therefore I think it is really hard to say what brings most long-term benefits in the general case. I've been through both rewrites that have practically killed the product, where refactoring would have been the right thing to do, and I've been through refactoring efforts that did little to pay back technical debt, even though that was the stated goal.
* You can tackle it in stages * You can try to ensure regression, feature parity, add tests * You can measure progress
You can do the same with rewrites, though I acknowledge it is harder to stage it.
A full rewrite is also a viable strategy. I see it as slightly more risky. Some rewrites are spectacularly successful. Some never make it. Whereas refactoring, by definition keeps exactly the same functionality but improves the code base for those that come after, normally in a time limited way. I can live with either path.
Indeed. I think we have to try to understand what makes either strategy viable. Refactorings is in my opinion the right strategy if
None of these are true for node-solid-server, as far as I can see. Point 3 is especially deadly, it is deadly both for refactoring and rewrites, but it leaves you with no option but to make big rewrites anyway.
It seems like we have a pretty good idea now of what solid-server should look like, so point 1 isn't there. I would myself not be advocating a rewrite if 1 was true, so one way to change my mind would be to point out that point 1 is still very true. Point 4 is something that I've challenged you on, we really need to know that. Embarking on a refactoring effort without knowing 4 is meaningless. But I haven't been on the project for very long, and I haven't seen all the code, but @RubenVerborgh has, and he can't see it either, so again, this is a very concrete challenge when discussing this topic that has to be met.
Some specific comments :
Maintenance fixes are still released as v5.0.x, but no more new features.
This sounds a bit like a code freeze to me. As a user, app dev, and occasional maintainer of solid systems, relevant to me is : how long would there be a feature freeze? My estimate would be a rewrite to take c. one year, and I'd be interested in the timelines of those that might like to work on this.
So, we're talking a feature freeze on the server, right? And the server isn't supposed to be evolving a whole lot anyway. The intelligence is supposed to be on the client side. So, the first question is: What kind of feature additions are you expecting from the server in near future? Secondly, would these features be feasible to add with the current server in a reasonable fashion?
We have two examples, delegation, which we used to have in the past seems infeasible to add because it touches so much, and different backend storage, which are also being worked on, but it also seems infeasible. So, what other features would you expect to see that can be done?
I think that we should maintain an open ear to the community, so if anybody contributes new features, we could release a 5.x.y, but they should then be aware that this code may not make it further, and I hope that the community would take greater interest in the rewrite effort.
I'm usually wrong about time estimates, I'll give you that. Multiply by a factor of π... ;-) But the thing is, the spec is there and it is mostly quite clear, and in environments I know, I have done very similar stuff quite quickly. Certainly not a year. I wouldn't count it in months, I think, but it also depends a lot on how much people who have been through the learning process are able to be involved.
As soon as the server has feature parity is achieved
What would be the metric for achieving this. We dont have full test coverage on the server side, for example for command line switches (not even sure how to do that!). And barely any test coverage for apps. How will we ensure apps, and also servers, don't break upgrading from the old server to the new?
We need to create a suite of systems tests first. A test suite that anyone can run to verify their server. We need to do that whether we do a rewrite or a refactoring effort. I think it is the first project we should embark on once 5.0.0 is released.
With that, progress on a rewrite can be measured just as well as a refactoring effort can be measured.
We need to create a suite of systems tests first. A test suite that anyone can run to verify their server. We need to do that whether we do a rewrite or a refactoring effort. I think it is the first project we should embark on once 5.0.0 is released.
:+1:
Just an observation :
Nextcloud on board would require another PHP server that is suited for their purpose
I'd say putting effort into this would be a bigger win for solid. Nextcloud has 25 million users and is growing fast. They have stated an intent to want to work together. Those windows of opportunity do not necessarily stay open indefinitely.
Create a design and implementation and create and implementation in PHP, and if apps work on it, then the proof of concept is successful and we do the same for node solid server. At the same time work on fixing the last few bugs in node solid server (which surround auth) leading to the "it just works", state that @kidehen has polularized!
Several advantages here :
As I say, it's nor my decision. This is a choice for inrupt. I'm happy with any path. But that is the route I would find tempting.
We would need a proposal for concrete stages from someone. My opinion is that the code is too tangled for that.
What I would suggest is to work only on auth, after 5.0.0, until there is consensus that we have got to "it just works", and at the same time create a documentation of existing interfaces. That would lower the risk of a potential rewrite. This is just food for thought, I dont want to get in the way of consensus!
@melvincarvalho Note that you haven't answered the question though. We still need a concrete place to start in order to be able to refactor.
What I would suggest is to work only on auth
But that's the issue: there's no such thing as only working on auth.
An important piece of context for this thread, which we perhaps should have made explicit, is that the current codebase is end of life. So "just work on X" or "just work on Y" is a fallacy, since working on X or Y requires touching large parts of the code, even for small X or Y, and certainly for major ones like auth.
So it's either refactor or rewrite; continuing as-is is not a realistic option.
the current codebase is end of life
I think it is normal to mark something as end of life, a period of time after, a replacement is available.
"End of life" as in unmaintainable.
I would like to use the Solid server or parts of it in some of my projects. With the current architecture and the lack of interfaces, this is very complicated. I doubt it's possible to fix that just with a refactoring. I also expect a rewrite could make progress much faster, as multiple people could work on different modules more conflict free.
We still need a concrete place to start in order to be able to refactor.
Replying to this for completeness.
I agree it is a challenge to fix bugs, part of this is because solid by design has many moving parts.
I really liked the idea from @bergos
The WebID authentications should be implemented as Passport.js modules as it's the de facto standard for node authentication modules.
So, possibly 6 steps :
This will need to be done probably with either route.
Evaluate the extent of client side changes to create a cleaner pluggable system
Implement the server side, with unit tests
I think that's about all I have to say on this issue. I'll aim to use 5.0.0 when it comes out, and try fix any bugs. Looking forward to following progress on v.next!
@melvincarvalho There must be a misunderstanding: the client is neither affected by not involved in any of this, so steps 2/4/6 are not a logical part of a rewrite or refactoring.
I would like to use the Solid server or parts of it in some of my projects. With the current architecture and the lack of interfaces, this is very complicated. I doubt it's possible to fix that just with a refactoring. I also expect a rewrite could make progress much faster, as multiple people could work on different modules more conflict free.
I recall @msporny expressing interest in some parts of Solid stack, not sure if for https://github.com/digitalbazaar/bedrock or something else. I think contributions from Digital Bazaar team to some common modules would come as a great help.
The WebID authentications should be implemented as Passport.js modules as it's the de facto standard for node authentication modules.
It looks like bedrock has one for HTTP Signatures https://github.com/digitalbazaar/bedrock-passport
I recall @msporny expressing interest in some parts of Solid stack
We expressed interest more at an architectural and "shared goals" level. Much of the thread above talking about an architecture for Solid is effectively describing Bedrock and how we built our "Solid Server"-like system. We'd be happy for Solid to use Bedrock and build on top of it (Bedrock can be used license-free for research purposes). That said, I would expect that some in the Solid project may not like that approach for a variety of different reasons (lack of a "true" open source license being one of them).
Digital Bazaar does create quite a number of open source packages in node.js that the Solid project is welcome to use (node.js libraries for: JSON-LD, Linked Data Proofs, Linked Data Signatures, HTTP Signatures, OCAP-LD, DIDs, Verifiable Credentials, etc.).
In any case, just responding to @elf-pavlik since he tagged me in this issue.
We still need a concrete place to start in order to be able to refactor.
Replying to this for completeness.
I agree it is a challenge to fix bugs, part of this is because solid by design has many moving parts.
I really liked the idea from @bergos
The WebID authentications should be implemented as Passport.js modules as it's the de facto standard for node authentication modules.
So, my view is indeed that we should indeed by adopting frameworks for essential parts. As frameworks also tend to come and go (like, myself, I was surprised to see that jQuery was completely out), we need to approach them with a critical mind, but not using them leads us in the much more dangerous pit of NIH, so, I'm all for it.
However, that is hardly suited to inform a "refactor" or "rewrite" decision on its own. The devil is the details, and as I said, adapting code to frameworks is one of the key factors that make refactoring interesting, but it depends on how far the current code is from where it needs to be to fit a given framework. I don't know that, but grepping the source tree for "auth" finds the word in 159 files, and that scares me, most code should be able to safely assume that the agent has been authorized. So, yeah, that's probably bad metric.
I think this is part of the architecture work that needs doing, and yes, I think we need proofs, verifiable identities and that kind of stuff too, so looking into @msporny et als code should also be a part of that.
@melvincarvalho There must be a misunderstanding: the client is neither affected by not involved in any of this, so steps 2/4/6 are not a logical part of a rewrite or refactoring.
@RubenVerborgh thanks for clarifying, got it! I just added this part in because it was a semver-major upgrade (I've added that tag).
Have you considered Go as the language of choice?
@adriangrigore There is an older Go implementation (https://github.com/linkeddata/gold), but the benefit of JavaScript is that we have isomorphic code between the client and the server. For instance, the module performing RDF patches then behaves exactly the same on the client or server.
How about extracting/expanding the test suite to turn it into a validation suite for any Solid server? It's sure to be useful for the soon-to-be-exploding Solid ecosystem. It can start out as 'documenting' the way the current implementation works and then expand to make sure all the protocol's edge and corner cases are well handled.
How about extracting/expanding the test suite to turn it into a validation suite for any Solid server?
Yes, I think that is something that we should do as one of the first things we do after the release of v5.0.0.
I'd suggest using https://github.com/nestjs/nest (solves the architecture problem and uses express under the hood)
@marcus-sa It seems that Nest is a framework to build Web applications. The Solid server is a personal data store. Wouldn't this be a mismatch?
@RubenVerborgh yeah you're right, just looked through the codebase quickly and saw that you were using express, hence my suggestion.
Otherwise you could use this https://github.com/marcus-sa/one which uses the same module architecture principles as Angular and Nest, but where components and controllers are obsolete and not bound to the framework itself and using https://github.com/Inversify/InversifyJS under the hood as IOC making it (one) really extensible.
Unfortunately it (one) lacks documentation at the moment.
I'll throw my hat into this ring. :) I've made only very small contributions to this project but I too have found myself frustrated by the code. I chalked it up to my dislike of and inexperience with Javascript, but it seems others feel the pain too.
Being a proponent of statically typed languages I see two strategies that could be used:
A couple of suggestions.
For balance, I'd recommend stake holders reading this (rather opinionated) classic. Posted without comment.
https://www.joelonsoftware.com/2000/04/06/things-you-should-never-do-part-i/
@kjetilk I think may be premature to label this as 6.0.0, because we dont know if the 5.0.0 API will change before "this" lands. Suggest, keeping it as semver major, and giving it a codename or calling it v.next.
Less controversial I think is that if we are going to follow the sever spec (which we dont have to) after 1.0 the API should be fully documented.
Then any changes to that API should be the starting point of a semver major upgrade. I'm unsure we have done that, but it would seem to be a good starting point?
For balance, I'd recommend stake holders reading this (rather opinionated) classic. Posted without comment.
Let me just pull out a quote for reflection then :wink:
It’s important to remember that when you start from scratch there is absolutely no reason to believe that you are going to do a better job than you did the first time.
I think may be premature to label this as 6.0.0, because we dont know if the 5.0.0 API will change before "this" lands.
You're right from an API perspective, but a rewritten or refactored version should probably still have a semver major change for safety reasons. That said, happy with v.next.
Less controversial I think is that if we are going to follow the sever spec (which we dont have to) after 1.0 the API should be fully documented.
Oops, I did _not_ follow that for many of my projects 😮 We should probably aim for the spec being the documentation of the HTTP API. (But there's also the JS API then.)
For balance, I'd recommend stake holders reading this (rather opinionated) classic. Posted without comment.
Let me just pull out a quote for reflection then wink
It’s important to remember that when you start from scratch there is absolutely no reason to believe that you are going to do a better job than you did the first time.
Ah, but that is the essence of what I've been saying all along. They were coding in an unknown, changing world, we are coding in a known world. That's a huge difference. And it is the difference that makes that statement wrong. Netscape folks were coding to a world that was changing at a tremendous pace.
I don't know how long it'll take to rewrite it, but what I'm pretty sure is that it will take several years to get the current code into shape, and I'm pretty sure we would have changed every single line in the process.
So, it is an unfortunate fact that we do not have those who learned those lessons in the group anymore. That's a problem. I acknowledge that.
Consider this analogy: What we have now is a pyramid. It has been built with massif effort, with one architectural element: The Rock. Rocks are stacked upon other rocks to form the shape of a Pyramid. What we want is something like an Eiffel tower. Eiffel actually architected the tower based on a few thousand more years of knowledge in engineering, based on thousands of years of knowledge in physics. With that, he knew that he didn't need much material, and he could mix many architectural elements so that the tower would stand, in spite of being a much less massive construction. What might want to think that we want to have something that looks exactly like the great Pyramid of Giza, but we don't, we know that there are several things that we cannot efficiently build on the current architecture. We need something that has a top and four corners, though, that's given by the Solid spec.
Rewriting the server amounts to trying to remove the rocks that aren't necessary, based on thousands of years of knowledge, until it looks like an Eiffel tower, while making sure that it doesn't fall over in the process. This is how I understand it, and I believe that the conjecture that setting out on this path is going to be less expensive than just building the Eiffel tower is a pretty bold conjecture that needs substantial collaboration to be believable.
I think may be premature to label this as 6.0.0, because we dont know if the 5.0.0 API will change before "this" lands.
Sure. Lets not call it by a version number. v.next works for me.
I don't know how long it'll take to rewrite it, but what I'm pretty sure is that it will take several years to get the current code into shape
@kjetilk this comes across as an appeal to authority, concrete details of what we can fix, I find easier to digest.
Im looking forward to a concrete design proposal, which I hope would be an input to v.next. Typescript interfaces are a good start. Documenting the current API as per the semver spec also will help.
With my user hat on, I see the risk that the upgrade path from 5+ to v.next to have breaking changes. One thing that would be persuasive in the design to me would be a module system that could extend the functionality of node solid server -- Im not sure if that's feasible, though. Food for thought!
That said, happy with v.next.
@RubenVerborgh I think there's going to be a need for some things after v5 comes out.
Off the top of my head, having been to several years of meetings on this :
We've wanted the ability for the server to save who has written a file, if they are authenticated.
We've also wanted atomic updates to more than one file at once.
Just examples, there could be more.
@melvincarvalho What we are trying to argue here is that such seemingly simple features are extremely complex on the current codebase, and that implementing n such features on the old server will effectively cost more work (for a less qualitative result) than rewriting the server and implementing those features there.
Case in point: do check the code that was needed to implement account deletion, and the few quality guarantees we can give on that code.
Providing concrete details of fixes is something I'm asking from anyone who suggests a refactoring. I have argued above that I cannot find such fixes, hence my preference for a rewrite.
A new version of the server might have some breakage, but many more things are broken with the current server that are hard to fix.
@RubenVerborgh I hear you loud and clear!
My point is that there is a mix of arguments in this thread. Some appeal to authority, some anecdotal evidence, some burden of proof shifting, some proof by analogy, and some technical details.
What I'm saying is that the Eiffel Tower / Pyramid of Gaza metahpor, while colorful is less persuasive than the complexity of deleting an account, which is a great input.
Taking this case study. We can ask why was it so hard. This could well be the architecture or it could also point to the design. Do we have a link to the design?
Deleting an account on a server can literally be 1 line of code.
mv data/account deleted/account
Which has the advantage of being able to undelete, say if your password was stolen or someone had access to your browser while you were not at your device.
Lots of great points made. Im just saying that technical details are helpful for review and analysis.
One line of code doesn't work; there's still the account db and associated management.
How about first working on a validation suite for Solid servers in general? Then if one group wants to write a server from scratch, they have a target to aim at, and if another wants to whip this codebase into shape, they can rest assured they're not breaking anything in the process.
Such an implementation-independent test suite is part of the plan indeed, can also be used for servers in different languages.
Right, so I guess my suggestion is: table whether to fix or rewrite _this_ codebase (the overall question here) until there's a good validation test suite to allow _either_ to be done in a safe and sane fashion.
In theory, your right; in practice, we have finite resources and a high speed at which we need to move.
I think in the end who does the job calls the shots and contributors insight gives a good notion of that part of the equation.
@melvincarvalho Yeah, I appreciate your call for details, but we need to have 5.0.0 out before we can devote more time to v.next.
I don't have an opinion on whether nss should be refactored or rewritten (I'm refactoring it in my spare time, but I'll be equally thrilled if it's re-written). And I wholly agree that the general state of nss code quality makes it difficult to work with.
But I do want to push back against a given example.
@RubenVerborgh
Case in point: do check the code that was needed to implement account deletion
@melvincarvalho
Deleting an account on a server can literally be 1 line of code.
This is a great example of some of the misunderstandings in this discussion thread. No, it can _not_ be just 1 line of code. Like, not even in theory, under ideal architecture. And it's not even an issue of whether you're just deleting just the user account directory or also the database entry. There are a _lot_ of moving parts underneath, mostly having to do with user confirmation UI/UX.
Take a look at some of the suggested implementation notes on the original issue. (And that's not even a full list, it's about half or two-thirds of the steps needed.) None of those steps really depend on architecture. And they can't be reduced in any significant way. Like, you'll always need to add the delete button ui. (and the corresponding routes / controllers for it). You'll always need to prompt for confirmation. You'll always need to generate and store a one-time token. And generate a confirmation email. And implement a confirmation screen from that link. And only _then_ you get to deleting the actual account.
So I want to ask, @RubenVerborgh - what are we meant to conclude from looking at the code for account deletion? Do you think that once NSS is refactored or re-written, the code will be significantly different, like by orders of magnitude?
No, it can _not_ be just 1 line of code.
Indeed: there's the UI, storage, accounts, and tests for all of these.
what are we meant to conclude from looking at the code for account deletion?
The single-responsibility principle is violated in many places. Too many moving pieces.
https://github.com/solid/node-solid-server/pull/858/files took 20 changed files and over 1,000 new lines of code. Duplication of existing code because insufficiently modular. Cannot nearly be tested as well as we want to. No one dares to state that it really works in all cases, will never delete files it is not supposed to, or otherwise corrupt or endanger a running server. No one knows what existing functionality we broke by implementing it. We do not have the slightest idea about the security implications.
Deleting an account on a server can literally be 1 line of code.
This is a great example of some of the misunderstandings in this discussion thread.
Yeah, I just didn't feel obliged to point that out. There's a lot of stuff going on under the hood.
However, I'm pretty sure somebody has already written code that does account deletion. There should be a framework somewhere will such things are already implemented, and an architecture should just wrap this with minimal code.
(Ugh, apologies for the double-post. Github is acting up for me today.)
took 20 changed files
Ok, so, let's take a look at the account deletion PR. There's 20 files affected, which may seem like a lot for something as "simple" as account deletion. But let's take a look at what files they are. 5 of those are UI template updates (and one email template). 4 of those files are unit tests. And 2 are (not quite sure why) a package.json and package-lock updates. So we're already more than halfway through those 20 files. And the remaining 9 (8, really, since the debug.js edit is optional) are route and controller code for the 3 different kinds of requests necessary for the UI for account deletion.
So, just for my own calibration, how would these changes be different under ideal architecture?
And the remaining 9 (8, really, since the debug.js edit is optional) are route and controller code for the 3 different kinds of requests necessary for the UI for account deletion.
Point taken, I didn’t pick the best example regarding number of changes.
Other points still stand though (but happy to be proven wrong).
A better example with regard to number of changes is:
“Solid, please don’t assume that extensionless files are Turtle.”
Seems like a simple enough things.
After having created all of the rules that govern extensions and MIME types
into one file (https://github.com/solid/node-solid-server/pull/643),
any guess on how many files we’d have to change to make that work?
I honestly have no idea, but it’s 9 and counting, and I am not even near a quarter of the way:
https://github.com/solid/node-solid-server/compare/feature/wire-legacy-resource-mapper
And as I’m making more edits, things are progressively becoming much harder
to the extent that I don’t know whether I can finish this.
And we’re talking about a really simple change here, conceptually.
@rubenverborgh ohh yeah, no argument there.
That kind of stuff is a _nightmare_ to change. Wholly agreed.
One line of code doesn't work; there's still the account db and associated management.
Why is this a requirement? Surely it was a design decision. And one which made the patch more complex. Is there also a requirement to delete all entries in the server back ups? Point 1 - design can influence complexity, as well as architecture!
Similarly careful reading of my comment shows that I was referring to the server side work, not the client side UX which is a different set of files, more than half the code. Point 2 the complexity around the server work in this case has been over stated.
no one dares to state that it really works in all cases, will never delete files it is not supposed to
Well, if you move to a deleted folder, you lower the risk of arbitrary deletions, and also what about the user when they want to reverse the deletion?
These technical details are good inputs to v.next, and see where good design, refactoring, and rewrite can help.
Why is this a requirement? Surely it was a design decision. And one which made the patch more complex.
Ah no, this is exactly the point! Can you prove that the server doesn't blow up if its accounts table is in an inconsistent state? Or when a folder isn't there when it expects it to be there? That a user doesn't get to see another user's files when their folder is deleted? How do you test such guarantees? Which (_n_) component(s) provide(s) these guarantees?
I was referring to the server side work, not the client side UX which is a different set of files, more than half the code
The server still needs API calls to listen to the client. And the server serves the client, currently. And the served assets aren't tested. Or transpiled. Or validated.
Well, if you move to a deleted folder, you lower the risk of arbitrary deletions
Do we really? What happens if, when a delete is in progress, the user writes to a folder? Does the delete fail? How do we detect a semi-move? How do we recover? What portion of the server do we need to adjust to make that move happen? Do we need a daemon? Is the server unresponsive to requests from other users during the move? How do we test that?
These technical details are good inputs to v.next, and see where good design, refactoring, and rewrite can help.
Yes, but this is not the point of this thread. The point is to find evidence for or against a refactor or rewrite.
The evidence I aim to bring with this post is that the above questions are really hard to answer on the current codebase, and that this is a problem.
Ah no, this is exactly the point! Can you prove that the server doesn't blow up if its accounts table is in an inconsistent state? Or when a folder isn't there when it expects it to be there? That a user doesn't get to see another user's files when their folder is deleted? How do you test such guarantees? Which (n) component(s) provide(s) these guarantees?
@RubenVerborgh you can test this using our dev server solidtest.space -- remove a directory and then try logging on. The server should not blow up by removing files, because solid was designed to remove files. You can also read the code and write a degree of automated test.
The urgent requirement in this case was to remove email address and name from public publishing. The solution was more complex that solving that sole issue.
Do we really? What happens if, when a delete is in progress, the user writes to a folder? Does the delete fail? How do we detect a semi-move? How do we recover? What portion of the server do we need to adjust to make that move happen? Do we need a daemon? Is the server unresponsive to requests from other users during the move? How do we test that?
mv (move) is an atomic operation of the servers we are using, and can be reversed. Recursive delete is a more difficult and dangerous proposition, for all the reasons you point out. The difficulty was on the client side. But this thread was about the server side.
So, I think we've debunked the hyperbole associated with this issue. ie that the delete account code was bloated due to server architecture. No, it was bloated due to a combination of factors, such as design decisions, client side code, user of server side templates, writing and exposing a new API workflow.
We need to create a suite of systems tests first. A test suite that anyone can run to verify their server. We need to do that whether we do a rewrite or a refactoring effort. I think it is the first project we should embark on once 5.0.0 is released.
@kjetilk I think this thread has been already useful in that we can do a better stock take after 5.0.0 is out with some good case studies. To documenting the whole API of 5.0.0, and then writing the tests is going to be a fantastic input. And so is the test suite mentioned above. It's also great that case studies are documented in this thread. I still think the rewrite is the high risk option, because there's not regression guarantee, so the burden of proof shifting is not yet something I buy into. Good discussions, tho!
@RubenVerborgh you can test this using our dev server solidtest.space -- remove a directory and then try logging on. The server should not blow up by removing files, because solid was designed to remove files.
Note that the deletion mentioned here is _not_ covered by the Solid spec, because it's a deletion above the level that the regular HTTP interface can access, namely the folder / itself. The user does not have an ACL on that folder.
You can also read the code and write a degree of automated test.
Both of which are really hard, because of the codebase state and architecture.
The urgent requirement in this case was to remove email address and name from public publishing.
People demanded account removal really.
So, I think we've debunked the hyperbole associated with this issue. ie that the delete account code was bloated due to server architecture.
The "hyperbole" in this issue was about much more than bloat, but about testability, stability, etc. Note also that the issue is not just about architecture, but state of the codebase.
No, it was bloated due to a combination of factors, such as design decisions, client side code, user of server side templates, writing and exposing a new API workflow.
All of which relate to the state of the codebase.
I still think the rewrite is the high risk option, because there's not regression guarantee, so the burden of proof shifting is not yet something I buy into.
You are arguing for a refactoring, and your argument is based on the fact that such a refactoring is feasible and not high risk. However, crucial to a refactoring is being able to set a concrete first step of something that can be refactored. I have worked on the server code intensively, and I can—honestly—not identify such a step that is not an in-place rewrite.
So yes, I am shifting the burden of proof on the existence of a meaningful refactoring to you, because you assume that such a refactoring exists. Any proof that such a refactoring exists, and will not be high-risk, will strengthen your case. Any absence of such a proof (in presence of the evidence of non-feasibility we have given above), or evidence against the validity of the original points made at the top of this thread, leaves that claim unsubstantiated.
You are free to conclude that a rewrite is the high-risk option, but we can only share that conclusion if you show us that a refactoring is not. I'm saying no meaningful refactoring path exists because of https://github.com/solid/node-solid-server/issues/788#issuecomment-423354608. If you say it does, the burden of proof is yours.
@melvincarvalho Yeah, I gotta echo @RubenVerborgh 's sentiment here. You have done nothing to document your assertion that a refactor is the lower-risk operation, because you have not provided any evidence for how and where refactoring can be done. For the record, I think both are high risk operations, but for different reasons: Refactor is high risk because the known volume of work to be done is very high and difficult, rewrite is high risk because we do not have the people who learned the lessons from the previous version on the team for the next version. However, I don't see that we can gain much from refactor, but we have a lot to gain from rewrite.
You are arguing for a refactoring, and your argument is based on the fact that such a refactoring is feasible and not high risk
I'm not arguing that, because it is self-evident. Refactoring, by definition, cleans up code, without changing functionality.
I hear your view that you consider it impractical, but it's not the only view that can be had.
During the process of getting to 5.0.0 I've identified things that can be refactored, such as the get handler to fix the bug where HEAD was showing the wrong thing. We talked about that on gitter. So the idea that refactoring in stages is impossible seems to be an appeal to authority right now -- which is fine!
There will be a lot greater visibility on this after 5.0.0. @RubenVerborgh I think you've ruled yourself out in coding this, if im not mistaken. I wont either be working on the v.next code base. But there certainty will be a lot of work needed on the 5.x, which wont be able to wait for v.next. So to an extent we have to look at the feasibility of this with an eye or resource allocation. Still not buying in to the burden of proof shifting, sorry!
it's not the only view that can be had
Thankfully.
And I'm willing to consider any other view, given evidence for its validity.
your argument is based on the fact that such a refactoring is feasible and not high risk
I'm not arguing that, because it is self-evident. Refactoring, by definition, cleans up code, without changing functionality.
It's not self-evident.
It's not because we're not changing functionality, that it is suddenly low-risk. I challenge you to refactor any of the node-solid-server files from callbacks to promises or async. Zero functionality change, but tell me again it's low risk when your're done 😄
It's not because we're not changing functionality, that it is suddenly low-risk. I challenge you to refactor any of the node-solid-server files from callbacks to promises or async. Zero functionality change, but tell me again it's low risk when your're done :smile:
:+1:
@melvincarvalho looking at all the 18 commits you made to this repo it looks mostly like release commits, merging PRs and documentation plus few small feature / bug fix commits.
I think that if other contributors shift their focus to rewrite they will still review your PRs where you focus on refactoring existing code and adding new features. This way no one stops you from submitting new features and improving shape of existing codebase. I really think that in the end we can build some shared understanding by talking the talk but walking the walk results in running code and that in software project matters the most.
Myself I have no contributions to this codebase but I would like to follow the rewrite closely and in that process maybe start finding some low hanging fruits to start contributing little by little.
It's not self-evident.
To be slightly clearer. I'm using what I think is the standard understanding of refactor.
Code refactoring is the process of restructuring existing computer code—changing the factoring—without changing its external behavior.
Certainly, as I stated, some refactoring may be impractical or not cost effective. But if itself is a refactor, it will not be risky, just time consuming. Otherwise it's not a refactor.
Hope that clear that one up. I'm going to bow out of this thread now, until we get to 5.0.0, at which point things will be much clearer.
Certainly, as I stated, some refactoring may be impractical or not cost effective.
Indeed, and an important part of the argument here is our evidence that refactoring of NSS seems to demand more work (for less result) than a rewrite.
But if itself is a refactor, it will not be risky, just time consuming. Otherwise it's not a refactor.
Nothing in the definition you posted precludes high risk.
Quite the contrary, your definition says "without changing its external behavior", but we currently have very limited ways of know whether that external behavior changes. It is very hard to make one modification in NSS for which you can guarantee external behavior does not change. As proven by the many regressions we have every time we fix something. Recall how 401 has been fixed more times than I can count already.
With a comprehensive test suite, we would at least know what we'd be breaking. But it wouldn't change the fact that we would.
Restructuring poses a high risk ("risk" as in failure to achieve the desired goals on time) if there is no good structure in place in the first time. Think of it as restructuring a house that was built without an architect: do you feel comfortable working on the second floor, not knowing what is below?
Based on all of the evidence posted in https://github.com/solid/node-solid-server/issues/788#issuecomment-423354608, we consider a refactor of NSS risky. If you change anything on the internal structure of NSS, you have no way of knowing how it affects the others (and external tests will not help you discover the cause, they only show breakage). Furthermore, a change in one component ripples through many other components. Fix one bug and create two more—and you'll only find out about them after you've deployed.
Such a code base is risky to refactor, because we don't know where to begin, and we don't know for every step we take how many other things will break.
I will accept any evidence that NSS would not be such a codebase as I'm describing above, but this thread so far only has evidence that NSS indeed is exactly that.
But if itself is a refactor, it will not be risky, just time consuming. Otherwise it's not a refactor.
Consequently, if we fail to identify a task on NSS that is _not_ risky, a refactoring of NSS does not exist? :wink:
I'm going to bow out of this thread now
Okay. When you return, please bring evidence about NSS being risk-free to refactor, for instance, by showing us a concrete component where we can start. One concrete suggestion for one concrete improvement will be the first evidence for the possibility of a refactor—evidence that we are still lacking up to this point.
Maybe we distinguish between improving code (which includes refactoring, but also restructuring certain parts of the code) and rewriting, instead of (just) refactoring and rewriting?
I should note that I'm for rewriting, as I think refactoring the current code base is going to be extremely difficult (that is, I think we would have to go through many rounds, and quite certain that it would take more time than a rewrite in the long run). A lot of it would actually be (hopefully) improving the existing code, as some parts are very difficult to test right now, and in those cases we cannot be 100% sure that changes are merely refactoring.
Just wanted to add this bit to the discussion in hopes that it will allay any concerns about diversion of attention from current server during rewrite or refactor efforts.
We are currently, and will continue to dedicate resources to sustain and stabilize the current server. It's what we have now, and will be until we have something better. And so in the dialogue here (which is all good and constructive) - keep in mind that no matter what path we choose, there's still going to be people dedicated in parallel to keeping the current codebase in good working shape so people can use it and get value from Solid.
With IPS up, we can close this issue :-)
Most helpful comment
We expressed interest more at an architectural and "shared goals" level. Much of the thread above talking about an architecture for Solid is effectively describing Bedrock and how we built our "Solid Server"-like system. We'd be happy for Solid to use Bedrock and build on top of it (Bedrock can be used license-free for research purposes). That said, I would expect that some in the Solid project may not like that approach for a variety of different reasons (lack of a "true" open source license being one of them).
Digital Bazaar does create quite a number of open source packages in node.js that the Solid project is welcome to use (node.js libraries for: JSON-LD, Linked Data Proofs, Linked Data Signatures, HTTP Signatures, OCAP-LD, DIDs, Verifiable Credentials, etc.).
In any case, just responding to @elf-pavlik since he tagged me in this issue.