Repository navigation
Is refactoring or rewriting the most efficient and cost-effective way to realize a high-quality and robust codebase for NSS? #788
Description
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.
Activity
kjetilk commentedon Sep 20, 2018
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.
1. Major issues with the current code-base
1.1 Lack of architecture and interfaces
Issue
The server had no upfront architecture, and the current code has organically grown.
Consequences
The resulting code structure is very difficult and hard to maintain. Changing components is hard, for instance:
1.2 Components use in-depth knowledge about each other
Issue
Consequences
Example
1.3 Violations of single-responsibility
Issue
One piece of knowledge/functionality is encoded in multiple places
Consequences
Example
The mapping between a URL and the filesystem is implemented in multiple places.
1.4 Abundance of global state
Issue
Consequences
Examples
As a result, some components might behave incorrectly/insecurely because the global state is abused for local purposes.
1.5 Lack of testability
Issue
Example
Consequences
1.6 Varying coding styles and quality (minor)
Issue
Example
Consequences
Note
While this issue can be fixed through refactoring, the lack of unit tests makes it difficult to assess the correctness of the refactoring.
2. Conclusion: a rewrite is needed
Given all of the above, we conclude that:
3. Proposed timeline
4. Approach to new server development
In order to avoid encountering the same problems of v4/v5 again, we should:
If something is not unit-testable, this points to a problem with the code structure / architecture.
RubenVerborgh commentedon Sep 20, 2018
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.
elf-pavlik commentedon Sep 20, 2018
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 👍 for a clean rewrite.
kjetilk commentedon Sep 20, 2018
I'd like to add some perspectives:
What is hard
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, solid/solid#152 may be such an issue.
This possibility should be balanced against other problems with the codebase, however.
Experiences with refactoring vs rewrites
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).
Experiences with NSS so far
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.
We need to know what it costs
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.
Conclusion
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.
kjetilk commentedon Sep 20, 2018
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.
nicola commentedon Sep 21, 2018
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.
melvincarvalho commentedon Sep 21, 2018
@nicola thanks for the feedback! Do you have any thoughts regarding the approach
Or a combination of the above?
kjetilk commentedon Sep 21, 2018
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.
bergos commentedon Sep 21, 2018
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.
melvincarvalho commentedon Sep 21, 2018
@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 :
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.
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!
RubenVerborgh commentedon Sep 21, 2018
nicola commentedon Sep 21, 2018
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
RubenVerborgh commentedon Sep 21, 2018
@nicola Thanks, any assessment on completeness besides what the README says?
44 remaining items
dmitrizagidulin commentedon Oct 22, 2018
@RubenVerborgh ohh yeah, no argument there.
That kind of stuff is a nightmare to change. Wholly agreed.
melvincarvalho commentedon Oct 22, 2018
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.
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.
RubenVerborgh commentedon Oct 22, 2018
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?
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.
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?
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.
melvincarvalho commentedon Oct 23, 2018
@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.
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.
@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 commentedon Oct 23, 2018
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.Both of which are really hard, because of the codebase state and architecture.
People demanded account removal really.
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.
All of which relate to the state of the codebase.
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 #788 (comment). If you say it does, the burden of proof is yours.
kjetilk commentedon Oct 23, 2018
@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.
melvincarvalho commentedon Oct 23, 2018
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!
RubenVerborgh commentedon Oct 23, 2018
Thankfully.
And I'm willing to consider any other view, given evidence for its validity.
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 😄
elf-pavlik commentedon Oct 23, 2018
👍
@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.
melvincarvalho commentedon Oct 23, 2018
To be slightly clearer. I'm using what I think is the standard understanding of refactor.
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.
RubenVerborgh commentedon Oct 23, 2018
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.
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 #788 (comment), 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.
Consequently, if we fail to identify a task on NSS that is not risky, a refactoring of NSS does not exist? 😉
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.
megoth commentedon Oct 24, 2018
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.
justinwb commentedon Oct 24, 2018
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.
kjetilk commentedon Sep 25, 2019
With IPS up, we can close this issue :-)