900 move diagrams #922
900 move diagrams #922
Conversation
| import java.io.PrintWriter; | ||
| import java.nio.file.Path; | ||
|
|
||
| public class GraphDiagram implements DiagramElement { |
| @Override | ||
| public String html() { | ||
| outputDir.toFile().mkdirs(); | ||
| return writeDiagram(writeDot()).html(); | ||
| } |
There was a problem hiding this comment.
Probably outside of scope, but I strongly dislike what's going on here. Especially the side-effects.
There was a problem hiding this comment.
If I invoke something like new GraphDiagram(/* ... */).html();, I expect to receive the html representation of the diagram viz., html markup to display a diagram. I don't expect to also create a directory and a file containing the graph. Just the number of responsibilities is enough of a problem, and it's made even worse by I/O. On top of this, the details are private and thus invisible - though they leak out through the exception - yet the coupling is still there (only hidden).
There was a problem hiding this comment.
Yeah, I would say that this is a bit out of scope.
I'm conflicted, since from one point of view I want the object to solve the task at hand, produce html that can show a graph as a diagram, the implementation specifics shouldn't be important.
I mean I don't want to micromanage my objects. Additional collaborators could be introduced but they wouldn't reduce the accumulated side-effects, it would reduce the immediate responsibility of the "top" object".
Aren't details supposed to be hidden, you shouldn't know more about me than necessary.
I wouldn't say that having an implementation throwing unchecked exceptions is a information leak, since the application wouldn't/shouldn't know about that. Test might, since they are implementation specific.
But if we play with the thought that we use VizJs in the browser.
new BrowserDiagram(/..../) could just be replaced this implementation.
The html markup would be something cool that triggers VizJs to render the diagram.
There was a problem hiding this comment.
Just want to be clear about one thing.
In my head DiagramException is a sub exception and not a sibling.
The dot and diagram packages should be sub-packages to the html package.
There was a problem hiding this comment.
So something like
In essence, yes. Of course, names and assignment of responsibilities may end up looking different.
What is the distinct difference between hidden and obscured in this case?
When I ask GraphDiagram for the html() I should not know how the result came to be (produced, retrieved from a local cache, etc.) If upheld, then we have the case of hidden. What I mean with obscured is when this isn't upheld.
If I were to monitor the file system when doing the call, I would see the file be created. And perhaps I haven't given it permission to do that, or I decide to remove the file before usage. So I can see it's details (and might even start relying on that information 😉 ). Same thing when we receive the exception.
In certain situations, this could be a security problem, but here and now the bigger thing is that many assumptions aren't explicit for no good reason, which increases cognitive load and lowers maintainability.
There was a problem hiding this comment.
When I ask GraphDiagram for the html() I should not know how the result came to be (produced, retrieved from a local cache, etc.)
Really strange sentence. When I'm doing something I shouldn't know how thing will have happened.
From UAP if you really want to go the UAP path. Violation to UAP would be to have the same result but two different notations. writeFileAndGiveHtml() and calculateInMemoryHtml().
But much of this is due to the fact that you thinking about the caller. I can call both methods get the same results, so why should there be two different methods? Some argue that one should violate UAP since for instance the impact of calculating cane be significant and that the caller should be aware of this. Other suggest that it should be discoverable to the caller or just plain simple documentation.
If I were to monitor the file system when doing the call, I would see the file be created. And perhaps I haven't given it permission to do that, or I decide to remove the file before usage.
If I were to provide an application with and output folder. I would feel that we have a mutual agreement that the application is allowed to write to the folder, that I have assigned as an output folder. If the application can't write to said folder I would hope that it let's me know of my betrayal. If I start removing files, I'm betraying the application.
As for the TOCTOU
One general technique is to use error handling instead of pre-checking, under the philosophy of EAFP – "It is easier to ask for forgiveness than permission" rather than LBYL – "look before you leap" – in this case there is no check, and failure of assumptions to hold are signaled by an error being returned.
and might even start relying on that information 😉
You can always raise issues, but if you rely on anything that isn't agreed upon, you're out of luck. Can't make everyone happy. An agreement can be made. So raise an issue I would say.
I think UAP exists to make that flexibility, as in this case. We can if we want, if we don't, we don't. Why we say we have an HTML report and not an HTML report in this format or structure, we have been vague on details to be more flexible. HTML is supposed to be processed by humans, humans are much more adaptable than XML parsing. We have versioned XML, since it's "more" machine readable.
But inspecting during or even after, seems really strange from a caller perspective. Is there an arbitrary line at filesystem?
If we just talk inspecting/observing the change/access; threads, heap, ram, cpu, network, database. There is no true hidden since almost anything can be observed.
In certain situations, this could be a security problem, but here and now the bigger thing is that many assumptions aren't explicit for no good reason, which increases cognitive load and lowers maintainability.
What is this something in the PR that is an issue, or something in the PR that could be a security issue in a totally different context? I just wonder if we are in the context of a CLI run by a user or a webserver responding with full stack-trace and message. Just the basic fact that stored/displayed information, if sensitive is a risk.
There was a problem hiding this comment.
Really strange sentence. When I'm doing something I shouldn't know how thing will have happened.
Almost. When I delegate a job to you, I will not try to retain any control over how you do the job.
If I were to provide an application with and output folder. I would feel that we have a mutual agreement that the application is allowed to write to the folder, that I have assigned as an output folder.
I agree completely. We have the same reasoning (though different granularity). I'm also talking about this in the context of GraphDiagram, specifically, having too many responsibilities. To put it differently, I'm arguing that a better design would be that GraphDiagram doesn't know about outputDir i.e., give the I/O responsibility to a different object.
Is there an arbitrary line at filesystem? If we just talk inspecting/observing the change/access; threads, heap, ram, cpu, network, database. There is no true hidden since almost anything can be observed.
It depends on which perspective you take (I guess we're well into the philosophical parts by now). Storage (whether file system or database) is intentional - a way of interacting with the world outside of the running application. RAM and CPU is the physical architecture of the computer. From another perspective, like side-channels, I wouldn't differentiate them as much.
What is this something in the PR that is an issue, or something in the PR that could be a security issue in a totally different context?
Yes, that the object behaves in a way that I would not expect is an issue in this PR - that I think should be addressed because of maintainability and testability. The security perspective is less relevant in this PR, because there isn't much to gain from performing an attack.
There was a problem hiding this comment.
Almost. When I delegate a job to you, I will not try to retain any control over how you do the job.
If that holds true, you shouldn't suffer from pre-conceptions about what it should or shouldn't do. You're invading it's privacy.
It's a lot different to talk about how it will do it's job and how it did it.
GraphDiagram, specifically, having too many responsibilities. To put it differently, I'm arguing that a better design would be that GraphDiagram doesn't know about outputDir i.e., give the I/O responsibility to a different object.
Introducing more collaborators doesn't change UAP argument you made. Since UAP isn't applicable to constructors.
Having different objects handling dependent I/O won't that just increase TOCTOU?
But by that argument the dot writing should be pushed down to Renderer/GraphProducer. You would just get the same implementation in Renderer.
If you would aske me what the issue is.
I would say that it's the Renderer that needs two files as input. With the DiagramElement interface, we can remove one of them, the target.
If Renderer would receive a Graph then we would just reproduce GraphDiagram but in Renderer.
But introducing a GraphFile that can write and return a path, and then possibly maybe create a "decorator"/wrapper for Renderer that returns Html and just receives a path and we have reduced the responsibilities of GraphDiagram. But won't affect UAP.
It depends on which perspective you take (I guess we're well into the philosophical parts by now). Storage (whether file system or database) is intentional - a way of interacting with the world outside of the running application.
We'll it interacts with the outside world but I would argue that it's more about statemanagement then actually interaction with the outside world. But I've seen integrations done over memory pipes, databases where better options exist if you want to interact with the outside world.
RAM and CPU might be the physical representation, but internal memory and processes/threads. IPC or even heap as more software based things.
From the point of side-channels, nothing is safe. It's just a matter of access the culprit has.
I'm sort of at a loss, I agree on the responsabilities. Just that it needs work in more places than this. But you're argument is that html() shouldn't touch the filesystem is, firstly required, secondly not really any of the callers concern. I'm arguing from the point of the interface not GraphDiagram specifically. As said before with collaborators, the call will still need to write the files and run either graphviz or vizjs.
So from an internal view the behavior of GraphDiagram would change but for a caller your initial issue mentioning UAP will remain.
There was a problem hiding this comment.
you shouldn't suffer from pre-conceptions about what it should or shouldn't do. You're invading it's privacy.
Fair enough and consider this view: The object is trying to "sell" my its service. If the quality is not good enough I'll look for a different seller (and perhaps leave a 1-star review). No privacy invaded and still a good reasons to avoid "expensive" I/O calls and user dissatisfaction.
Having different objects handling dependent I/O won't that just increase TOCTOU?
By making the dependency explicit (in code and code naming) the assumption is elevated to a contract (a pre-condition). So then we have an entirely different - much better - situation. In fact, we wouldn't need to do the "check" part at all.
I'm sort of at a loss, I agree on the responsabilities.
Then perhaps we should start there. If nothing else, it will make it easier to discuss when we can point to the code / a concrete example.
| DiagramProducer renderer = mock(DiagramProducer.class); | ||
| when(renderer.generateDiagram(any(File.class), any(File.class))).thenReturn(""); | ||
| when(renderer.getDiagramFormat()).thenReturn("svg"); | ||
|
|
||
| assertThat(new GraphDiagram(graph, renderer, outputDir, "agraph")).isInstanceOf(DiagramElement.class); |
There was a problem hiding this comment.
Clarify the purpose of this test
| when(renderer.generateDiagram(any(File.class), any(File.class))).thenReturn(""); | ||
| when(renderer.getDiagramFormat()).thenReturn("svg"); |
There was a problem hiding this comment.
Can and should be removed
| File summaryDir = new File(diagramDir, "summary"); | ||
| summaryDir.mkdirs(); | ||
| SummaryDiagram summaryDiagram = new SummaryDiagram(diagramProducer, summaryDir); | ||
| SummaryDiagram summaryDiagram = new SummaryDiagram(diagramProducer, outputDir); |
There was a problem hiding this comment.
Do we consider this a breaking change?
No description provided.