Refactor LDP API middleware by dmitrizagidulin · Pull Request #706 · nodeSolidServer/node-solid-server

dmitrizagidulin · 2018-06-12T15:42:49Z

Proposed LDP API refactor, based on @RubenVerborgh et al's http request flow.

Please give feedback early & often, to make sure we're on the right track.

(Part of ongoing PR #702)

dmitrizagidulin · 2018-06-12T20:36:37Z

+        /**
+         * Use cases to handle:
+         *  - LdpListContainerRequest, if target is an ldp container
+         *  - However, if Accept: header is HTML, check if index.html exists


It would be nice to be able to dispatch between "List container" and implicit "I'm requesting a container url but really get me its index.html/index.ttl" type of GET requests here, during parsing & constructing the Request object.

But that requires passing in a backend object (e.g. LdpFileStore), to check if said index.html exists. Which I was initially reluctant to do, in terms of separation of concerns (and only pass in the backend into the request.handle() method). But it seems like it might be a good idea to pass it in at this early stage.

Then maybe target should be parsed first before this decision happens. You should be able to use https://github.com/solid/node-solid-server/blob/v4.0.14/lib/resource-mapper.js for that.

Ah, I meant the resourceMapper comment above for this sub-thread.

dmitrizagidulin · 2018-06-12T20:52:28Z

+         */
+        request = LdpPutRequest.from({req, host})
+        break
+      case 'post':


Currently, the target path for POST is required to be a container. If it's not a container, solid-server currently throws a 405 Method Not Allowed error.

Should we continue to implement this behavior? I'm thinking yes, since POST to a resource doesn't seem to make much sense.

My other question is - should the "must be a container" requirement be signaled by a 405 error, or would it be more appropriate to return a 400 error instead?
(If the former, we need to make sure we return the Allow header per spec - "The response MUST include an Allow header containing a list of valid methods for the requested resource.")

Seems like a 405 case to me, that's the most specific one.

dmitrizagidulin · 2018-06-12T20:57:09Z

+         * Does NOT use `mkdir -p` semantics (does not create missing containers)
+         *
+         * Throws:
+         *  - 400 error if target doesn't end in a / (is not a container)


Actually, this doesn't happen currently (POST doesn't throw a 400 if target path does not end in a /). Since the dispatching between "create container" and "create plain resource" is done via the Link: type rel, the handler is lax about a missing / at the end (in fact, it adds it if it's not there).

RubenVerborgh · 2018-06-12T20:53:55Z

+
+      // perform the operation and return a response
+      await request
+        .handle({res, permissions, store: this.store, fetch: this.fetch})


Why are the permissions still passed?

I wasn't going to pass them initially. I added them because of the section in the architecture doc that said something like "after performing [a write] operation, check for a read permissions on that resource and return the contents if those are there, otherwise a 204 no content."

RubenVerborgh · 2018-06-12T20:58:13Z

+  async middleware (req, res, next) {
+    try {
+      // parse target, operation, (optionally) body, preferences, store authn
+      const request = LdpRequest.from({req, host: this.host})


The extra host parameter feels a bit clumsy; it's also incomplete because the protocol is not considered. Maybe we should more generally have an overrides parameter or similar.

how about if we call it serverConfig instead of host?

The name makes sense.

But actually, I think you just need to parse target first as follows:

have a resourceMapper private field, which is an instance of https://github.com/solid/node-solid-server/blob/v4.0.14/lib/resource-mapper.js (this already contains all the server settings)

set const target = this.resourceMapper.mapUrlToFile(…)

pass target to the LDP request constructor.

Alternatively, the resourceMapper can be passed to the LDP request constructor.

Actually, I think that LdpApi should take an ldpProvider as argument.

And if that ldpProvider happens to be a file-based LDP provider, then that will internally use resourceMapper.

Re 'private field' - heh heh, so, I don't really believe in private fields. I think Steve Yegge captures it perfectly with his essay: http://steve-yegge.blogspot.com/2010/07/wikileaks-to-leak-5000-open-source-java.html)

But that aside - so here's what's interesting about the resourceMapper (which I agree is useful).
It belongs to the backend, not to the API request, specifically only to the LdpFileStore (and not to an in-memory store or a Quad Store backend).

So the idea here is that the Request objects would only need to call this.store.exists(resource) and have the backend (if appropriate) worry about file mapping.

Ok. Will keep the store name.

I wasn't sure about passing in the host and fetch as separate parameters, either.
But they're not just needed in the store. The host and fetch are both needed by the ACL Checker. (And I was initially thinking that the host may be needed in the url parsing of target, but maybe not).

I suppose I can not pass them in, and just require passing in an already-initialized acl checker object (that will already have them)

So something like:

const target = this.targetParser.parse(request); const operation = this.operationParser.parse(request); const bodyParser = this.bodyParsers.find(request.headers['content-type']); const body = bodyParser && bodyParser.parse(body);

And there should just be no defaults for store and acl

Ok, copy that. Will remove :)

And hey, critical & demanding is perfect, that's what I want :) I'm happy to have LdpApi reflect the request-flow document (with some slight exceptions).

Ok, so let's take those one at a time.

const operation = this.operationParser.parse(request)

Is the operationParser() meant to take the place of the switch() statement? Cause that's essentially what it is, an operations parsing & factory method.

const target = this.targetParser.parse(request)

This makes sense, though I'm somewhat unclear on if we need any complicated parsing for the target url. (Which is different from url-to-fs mapping, a domain of the backend).

const bodyParser = this.bodyParsers.find(request.headers['content-type'])

I feel that this belongs in each operation subclass. Like, HEAD ops don't need parsing. Neither do PUTs and POSTS etc. But PATCH does.

Is the operationParser() meant to take the place of the switch() statement?

No:

The operation consists of the HTTP method, and a set of flags. Valid operations are GET, HEAD, OPTIONS, POST, PUT, PATCH, DELETE. Based on this method—and, in the case of PATCH, the parsed body—the read, append, write, and/or delete flags are assigned.

It's just parsing, really. (This shows that body should be parsed before operation though, so body can be passed as a param.)

This makes sense, though I'm somewhat unclear on if we need any complicated parsing for the target url.

Complicated enough that node-solid-server got it wrong in a couple of places :-)

The idea is to have this in a small separate component, so we can test all parsings independently. This includes URLs with spaces, encoded characters, etc. etc.

I feel that this belongs in each operation subclass.

We need to parse the body to determine the operation, and this is needed as input for the permissions checker. (And the operation subclasses do not [need to] know about permissions.)

RubenVerborgh · 2018-06-12T21:01:24Z

+        /**
+         * Checks to see if target exists
+         */
+        request = LdpHeadRequest.from({req, host})


Where do these come from? (I assume to be implemented still?)

Also, I wonder whether it's necessary to do the subclassing at this level, but it is an option. That said, we there's probably some common parsing to be done first.

I'm not sure either. What would be the alternative to operation-level subclasses?

(Agree about common parsing beforehand, in any case)

Well, actual operation-level subclasses 😄

These are request-level subclasses.
I would expect to have one LdpRequest which has an operation, and operations can then have different subclasses.

Not much difference I guess, slightly different way of modeling.

What do they say about naming being the hardest problem in CS? :)
I think it would be clearer if these subclasses are named e.g. LdpGetOperation, instead of LdpGetRequest -- cause they're modeling operations, not http requests. We leave request land in that switch() statement.

RubenVerborgh · 2018-06-12T21:02:12Z

+    let request
+
+    switch (req.method) {
+      case 'head':


Uppercase?

But also, I don't really like the switch. There should just be a handlers map somewhere, such that we can simply do handler = handlers[req.method].

Aren't Express request methods in lowercase?

I was torn between doing a switch() statement and a handlers map.

I think I went with it for those cases (like with the GET method) where additional logic may need to be done before figuring out a subclass, so that a simple map would not suffice.

Any particular reason for disliking switch, out of curiosity?

Keeping specialized knowledge in one place. The class implementing the switch currently needs knowledge on the internals of the constructors it creates, spreading out this specific knowledge (especially if adding that additional logic). With a map, all of that additional logic is inside the specific objects themselves, so only in one place.

Keeping specialized knowledge in one place. The class implementing the switch currently needs knowledge on the internals of the constructors it creates, spreading out this specific knowledge (especially if adding that additional logic).

So I think that's my point - the switch() statement enables specialized knowledge (as in the case of the GET operation - though that's not clear from the current code). (Though notice that it's not specialized knowledge of constructors, the subclass from() methods hide that knowledge).

Here, let me rename the Ldp*Request references to Ldp*Operation, to make the discussion clearer. (And take out the extra constructor params to LdpApi, as discussed). And add the illustrative code to the GET section of the switch().

So I think that's my point - the switch() statement enables specialized knowledge (as in the case of the GET operation - though that's not clear from the current code).

Yes, but it spreads it out across two places: the switch statement and the Ldp*Request constructor. The class containing the switch has to know about each of the subclasses. That's knowledge it doesn't need, and ties the implementation of this class to each of the subclasses. That's coupling that can be avoided.

Yes, but it spreads it out across two places: the switch statement and the Ldp*Request constructor.

No prob. Lemme replace the switch() with a mapper, let's see how it looks.

RubenVerborgh · 2018-06-12T21:02:28Z

+         *     (since we don't support conditional `If-None-Match` requests)
+         *   - has `mkdir -p` semantics (creates missing container hierarchy)
+         */
+        request = LdpPutRequest.from({req, host})


The above comments seem to belong in LdpPutRequest.

Yeah, heh, I plan to move 'em as soon as those classes get implemented.

RubenVerborgh · 2018-06-12T21:15:29Z

+  async middleware (req, res, next) {
+    try {
+      // parse target, operation, (optionally) body, preferences, store authn
+      const request = LdpRequest.from({req, host: this.host})


Actually, I think that LdpApi should take an ldpProvider as argument.

And if that ldpProvider happens to be a file-based LDP provider, then that will internally use resourceMapper.

RubenVerborgh · 2018-06-12T21:16:40Z

+      const request = LdpRequest.from({req, host: this.host})
+
+      // check that operation is permitted (throws error if not)
+      const permissions = await this.acl.allow(request)


This should take authentication and target, not the full request.

(One of the goals of the new request flow is to minimize the number of components that depend on HTTP stuff.)

RubenVerborgh · 2018-06-12T21:21:10Z

+    let request
+
+    switch (req.method) {
+      case 'head':


Keeping specialized knowledge in one place. The class implementing the switch currently needs knowledge on the internals of the constructors it creates, spreading out this specific knowledge (especially if adding that additional logic). With a map, all of that additional logic is inside the specific objects themselves, so only in one place.

RubenVerborgh · 2018-06-12T21:24:19Z

+  async middleware (req, res, next) {
+    try {
+      // parse target, operation, (optionally) body, preferences, store authn
+      const request = LdpRequest.from({req, host: this.host})


😱 Encapsulation is super important in object-oriented programming.

It belongs to the backend, not to the API request, specifically only to the LdpFileStore (and not to an in-memory store or a Quad Store backend).

Agreed, see my second comment (which was still pending when you commented).

dmitrizagidulin · 2018-06-13T00:31:45Z

@RubenVerborgh take a look at the latest iteration when you have a chance.

I'm getting stuck on the subject of body parsing (in trying to accommodate a lack of switch() statements :) ).

Specifically, here's the logic for parsers:

PATCH - uses a text() body parser for the request stream, then passes it to either a application/sparql-update or a text/n3 parser.

PUT & POST - either uses a multipart parser (such as busboy for multiple file uploads), otherwise doesn't use a parser and pipes the request stream straight into a file stream.

All others (head / get / options / delete / copy) - no parsers.

So that suggests to me only two options:

Have a switch() type statement somewhere like parseBody(), that goes through the above logic. or
Pass the request read stream into the Operation subclass (and have the relevant operations do parsing as needed)

RubenVerborgh · 2018-06-13T08:13:58Z

+   *
+   * @throws {Error} 405 If HTTP method not allowed / not implemented
+   */
+  async middleware (req, res, next) {


Maybe we can name this handleRequest?

Sure, we can rename it handleRequest - calling it middleware just references the common Express terminology.

RubenVerborgh · 2018-06-13T08:14:38Z

+
+const LdpOperation = require('./ldp-operation')
+
+class LdpApi {


Naming: perhaps LdpHttpHandler or similar?

Sure, renaming...

RubenVerborgh · 2018-06-13T08:16:50Z

+  async middleware (req, res, next) {
+    try {
+      // parse target, operation, (optionally) body, preferences, store authn
+      const operation = LdpOperation.from(req)


Given the comment, I'd expect

const { target, operation, body, preferences, authentication } = …

…but that's too much already. LDP shouldn't know about authentication, that seems the task of this class.

Heh, well, the comment was just referencing the architecture doc :) I removed the comment tho. (It just says // parse operation now).

…but the implementation was not in sync with neither that comment nor the architecture doc. Added a clarification about terminology here: #706 (comment), because we seem to indicate different things with “operation”.

RubenVerborgh · 2018-06-13T08:17:17Z

+      const operation = LdpOperation.from(req)
+
+      // check that operation is permitted (throws error if not)
+      const permissions = await this.acl.allow(operation)


Should take target and operation.

Why so? The target is already contained in the operation.

Not in the architecture's doc definition of “operation”, see #706 (comment)

RubenVerborgh · 2018-06-13T08:21:18Z

+
+  static async from ({req}) {
+    const { method, headers } = req
+    const authn = { webId: req.session.webId, credentials: {} } // <- bearer tokens go here


LDP should definitely not know about this. We should get authentication out of there, and definitely the very specific dependency on a session variable. This kind of state should only be accessed in a single place.

Sounds good.

RubenVerborgh · 2018-06-13T08:24:18Z

@RubenVerborgh take a look at the latest iteration when you have a chance.

Done.

So that suggests to me only two options:

There's a third option, which is documented in the request flow: a lazy parser.

So initialize a body parser based on the content type, but don't start the actual parsing until requested.

Parsing the operation will then only trigger an actual parsing of the body in case of PATCH.

dmitrizagidulin · 2018-06-13T13:05:35Z

@RubenVerborgh

There's a third option, which is documented in the request flow: a lazy parser.

Ahh, I see, I didn't quite understand that part, initially.

It still feels like the wrong place to set up the parser. It's too early, there's method-specific parser logic later on (which is more complicated than just the content-type).

Do you not think it makes sense to pass in the body stream (instead of a body parser) into the Operation?

RubenVerborgh · 2018-06-13T15:33:37Z

First of all, let's ensure we use the same terminology.

When I say “operation”, I mean “the HTTP method, and a set of flags”, as defined in https://github.com/solid/solid-architecture/blob/master/server/request-flow.md. Concrete examples are { method: 'GET', read: true } and { method: 'PATCH', append: true }.

When you say “operation”, you seem to mean the entirety of target, body, authentication, preferences and (what I call) operation.

I don't mind which terminology we pick, but we should be clear about that.

It still feels like the wrong place to set up the parser. It's too early

It is earlier than the current design, but this is on purpose.

The idea is that, in Step 2 of https://github.com/solid/solid-architecture/blob/master/server/request-flow.md, we parse everything necessary to determine the permissions. Then, in Step 3, we check the permissions. And in Step 4, we don't need to think about permissions anymore.

The method name by itself is insufficient to determine the permissions, as there are cases (specifically PATCH) where the name does not tell us whether read/write/append/delete permissions are needed.

We could make an exception for PATCH, which is what we currently do in the server. However, this specific exception breaks the layering of node-solid-server, i.e., it is specifically because of PATCH that the authentication in the current implementation cannot be a layer around LDP. Instead, we have to pass authentication all the way through to LDP, which leads to coupling, hinders testability, etc. It's messy, and we want to avoid that.

Therefore, https://github.com/solid/solid-architecture/blob/master/server/request-flow.md has been designed such that node-solid-server can be designed as a layered system. Which means a separate authentication layer around the LDP store, that this store does not need to know about. Which means that the request has to be parsed earlier. The philosophy is the request should be understood before we go to Step 3, and this does include parsing the body (to accommodate the specific case of PATCH—but if we make exceptions, we break the layered design).

And with the lazy design, we only parse when needed anyway, so we don't do too much work.

Bottom line: yes, we are parsing early, but this makes the overall design easier, with fewer dependencies, and hence more testable.

You are definitely free to disagree with that decision, and I am open to discussion as usual, but that discussion should probably happen on https://github.com/solid/solid-architecture/ if the goal of this PR is indeed to implement that document.

dmitrizagidulin · 2018-06-13T15:39:00Z

@RubenVerborgh - ok, I'm in the process of absorbing your points about definition of ~~iteration~~ operation, and early parser selection.

Edit: operation, not iteration :)

dmitrizagidulin · 2018-06-13T15:40:05Z

+    // ...
+    // if it is a container, check to see if index.html exists
+    const indexFile = url.resolve(targetUrl, '/index.html')
+    if (store.exists(indexFile)) {


@RubenVerborgh - the GET request dispatching logic here illustrates why I was thinking it makes sense to include the store (backend) param to Operation.from().

Mmm, I think the from is doing at lot here. Constructors shouldn't do actual work (and although a static method, from is acting as a constructor here).

I think the constructor should just create/parse the operation, and then we have a separate operation.perform(store) method.

The question probably is how much store-agnostic work an Operation can do.

Ok, I also believe that constructors shouldn't do actual work. But as far as I know, factory methods that set up and initialize the various things to pass to a constructor, are a valid strategy. So the from() method is just a factory method.

Now, as far as whether a store is involved in defining the operation - so my point here is that without a store, it's impossible to determine the operation. (Specifically, to decide whether this is a List Container op or a Get Op).

But as far as I know, factory methods that set up and initialize the various things to pass to a constructor, are a valid strategy. So the from() method is just a factory method.

It's never a black-and-white situation, but I perceive the static from method more as a de-facto constructor rather than a factory method:

I is a static method, and constructors are (kind of) static methods too. The idea of the factory pattern is precisely to not depend on static methods.

The idea of a factory is that users of the factory don't need to know how the factory creates its objects. Some other component has instantiated a factory with several constructor arguments, so when we call a factory method, we don't need to pass in lots of things over and over again (like protocol and host etc.). I.e., the nice thing of a factory is that it is an instantiated object, so we don't need to pass all the state on every method call.

There is no practical difference (from a caller's perspective) if we move the code currently in from to the constructor.

Now, as far as whether a store is involved in defining the operation - so my point here is that without a store, it's impossible to determine the operation.

Depending on the definition of “operation”, you do have a valid point here. Perhaps should be updated in the architecture document.

dmitrizagidulin · 2018-06-13T15:40:40Z

+   * @returns {URL}
+   */
+  static parseTargetUrl ({req, host}) {
+    const protocol = host.serverUri.replace(/:.*/, '')


@RubenVerborgh re our earlier discussion of whether or not the host param is needed - this usage of it while parsing the target is what I was thinking about.

The Operation should not probably not be performing this task; the target URL is also needed by the permission checker. (Also relates to our different definitions of “operation” though, where yours includes target but mine doesn't.)

I wonder if the name host is misleading.. It doesn't refer to the url hostname, it's a host server config object instance. Should I rename it serverConfig?

Perhaps, yes.

Also node that part of the relevant code and tests are here: https://github.com/solid/node-solid-server/blob/v4.0.14/lib/resource-mapper.js

ResourceMapper currently does two things: re-constructing the intended URL (with host and protocol), and mapping that URL to a file.

Since this first function is shared across all LDP stores, but that second function is unique to the file-based store, it might make sense to move that first code (and associated tests) to here.

Sounds good, lets move the relevant (url non-fs) functions in resource-mapper.js to solid-host.js ?

dmitrizagidulin · 2018-06-13T16:36:34Z

@RubenVerborgh

Ok, so as far as parsing the body (or at least selecting a lazy parser) before performing the operation, that's fine. I still think it's too early, but maybe it'll be fine - I'll go with it, and see if I run into any trouble in implementation.

Re definition of operation - yeah, I think that's exactly it. Our definitions are differing. I think that even if we create a simple operation object (that's just a method and some flags), there will still be a need to have "operation" like subclasses with specific logic. Like, we still need to distinguish between a glob operation, a list container, or a get action. And that's only possible with several things involved (like a store, etc).

RubenVerborgh · 2018-06-13T18:03:49Z

Re definition of operation - yeah, I think that's exactly it. Our definitions are differing. I think that even if we create a simple operation object (that's just a method and some flags),

I think we should rename in the architectural flow document. Perhaps instead of operation, should be method and requiredPermissions? And then another field, depending on whether it is a glob/container/…?

dmitrizagidulin · 2018-06-13T23:02:23Z

+ *  @return {LdpGlobOperation|LdpListContainerOperation|LdpGetOperation}
+ */
+class LdpGetOperation extends LdpOperation {
+  static from (options) {


@RubenVerborgh ok, so, I'm not super thrilled with this factory method, but it at least roughly fleshes out our semantics.

Indeed, it's quite alright!

dmitrizagidulin · 2018-06-15T16:45:00Z

+  async resource (target) {
+    const filePath = await this.mapper
+      .mapUrlToFile({
+        url: target.url, contentType: target.mediaType(), createIfNotExists: true


@RubenVerborgh Ok, so now we're at the stage of sketching the backend / LdpFileStore.
I suspect we'll need to refactor either ResourceMapper or LegacyResourceMapper.

The current behavior of mapUrlToFile() is to either 1) create the filepath using $ (non-legacy behavior) if createIfNotExists: true, or 2) Throw a 'File not found' error if createIfNotExists: false (which is always true if called via LegacyResourceMapper).

I'd like a slightly different semantics - to not throw a 'file not found' error, and just return "if the file existed this is what its path would be" in legacy mode.

I hope no major refactoring of LegacyResourceMapper is necessary; I tried to design it so it can be plugged in right away.

Let's ignore the new ResourceMapper for now and let's check the legacy one, which intends to capture the current behavior in Solid.

Throw a 'File not found' error if createIfNotExists: false (which is always true if called via LegacyResourceMapper).

You can't run into such an error with LegacyResourceMapper. It is a simple functional mapping between a URL and a filename, which, unlike ResourceMapper, does not depend on the filesystem. The legacy mapper (aka current behavior) is independent of whether the file already exists, it's a simple 1-on-1 mapping, regardless of disk contents. This changes with the ResourceMapper, which follows https://www.w3.org/DesignIssues/HTTPFilenameMapping.html instead.

I'd like a slightly different semantics - to not throw a 'file not found' error, and just return "if the file existed this is what its path would be" in legacy mode.

We should have that already. If not, you probably found a bug.

You can't run into such an error with LegacyResourceMapper

Ohhh I see it now. I misread. n/m.

dmitrizagidulin · 2018-06-15T18:13:03Z

Ok, so at this point, I should be able to hook up the new Delete handler to the existing ldp middleware, and it should (theoretically) pass the integration tests. (The delete operation is the easiest one, but still).

dmitrizagidulin · 2018-06-15T20:32:03Z

Oh hey, check this out! Turns out Express.js has implemented res.sendFile, with acceptRanges and everything. (Means we don't have to roll our own! At least for non-RDF resources).

RubenVerborgh

Definitely looks good overall.

RubenVerborgh · 2018-06-20T14:19:03Z

+   * @returns {object} Operation constructor params object
+   */
+  parseOperation (req) {
+    const { method, headers, session: { authentication } } = req


The knowledge about where and how the authentication is stored should probably live in another component.

Heh, sure thing. I was only including it here cause it was mentioned in the architecture flow doc.

RubenVerborgh · 2018-07-04T10:57:07Z

Re definition of operation - yeah, I think that's exactly it. Our definitions are differing. I think that even if we create a simple operation object (that's just a method and some flags),

I think we should rename in the architectural flow document.

In the architectural document, “operation” has now been split into “method” and “required permissions”: RubenVerborgh/solid-architecture@38e85ba

dmitrizagidulin · 2018-10-05T00:35:24Z

(rebased on latest develop)

rimmartin · 2018-10-18T19:18:14Z

Hi
Would Direct and Indirect container support be implemented better after this PR is merged or could these containers be implemented independent from this PR?

dmitrizagidulin · 2018-10-18T19:34:35Z

@rimmartin I'd recommend that it's implemented after this PR.

That said, there is no plan (that I know of) to support either Direct or Indirect LDP container, in Solid.

You might be best served by opening an issue on https://github.com/solid/solid and state your proposal (what would the semantics of Direct containers be, in the context of Solid? Solid is a web-ification of the file system, so what does it mean for a file to be stored in a non-Basic container? and so on.)

rimmartin · 2018-10-18T19:46:06Z

Thank you @dmitrizagidulin . I have a bit of a learning curve till this gets merged and LDP makes more sense to me

rimmartin · 2019-03-04T15:11:50Z

Hi @dmitrizagidulin, I suppose v5.0.0 getting to release is priority over LDP API dev.

but I'm needing to simulate/store symlinks; at this point it is the main priority for my project https://github.com/DataDriven-CAM/solid-fs which is implementing node fs for isomorphic-git and any other javascript project needing it.

This PR is based on master? Do you think it is yet compatible with the 5.0.0 branch?

dmitrizagidulin · 2019-03-04T15:18:42Z

@rimmartin It's not based on master currently, it was based on develop from several months ago (October). Let me see what it would take to base it on v5.

kjetilk · 2019-04-03T11:46:15Z

So, I suppose we can close this now, and you'd rather have it as a module, @dmitrizagidulin ?

jaxoncreed · 2019-04-16T17:00:57Z

@dmitrizagidulin Is this still being actively worked on?

dmitrizagidulin · 2019-04-16T17:01:56Z

@jaxoncreed Yes, but based on our previous conversation with you and the team, I'm going to move this PR to a separate external repo. So, it's ok to close. Thanks!

dmitrizagidulin · 2019-04-16T17:02:21Z

@kjetilk Ooof, my apologies, I somehow missed your previous comment! :)

jaxoncreed · 2019-04-16T17:03:18Z

@dmitrizagidulin Sounds good

dmitrizagidulin commented Jun 12, 2018

View reviewed changes

RubenVerborgh reviewed Jun 12, 2018

View reviewed changes

RubenVerborgh reviewed Jun 13, 2018

View reviewed changes

dmitrizagidulin commented Jun 13, 2018

View reviewed changes

Comment thread lib/api/ldp/api.js

dmitrizagidulin commented Jun 13, 2018

View reviewed changes

dmitrizagidulin force-pushed the ldp-api branch 3 times, most recently from ea305df to 44f46f4 Compare June 14, 2018 19:23

dmitrizagidulin commented Jun 15, 2018

View reviewed changes

dmitrizagidulin force-pushed the ldp-api branch from 5fa027f to 7a56579 Compare June 15, 2018 20:33

RubenVerborgh reviewed Jun 20, 2018

View reviewed changes

RubenVerborgh added the in progress label Jun 21, 2018

RubenVerborgh assigned dmitrizagidulin Jul 19, 2018

dmitrizagidulin force-pushed the backend_refactor branch from e0c627c to b80b708 Compare October 5, 2018 00:20

Refactor LDP API middleware

93343d2

dmitrizagidulin added 14 commits October 4, 2018 20:29

Add parseTargetUrl() to LdpOperation

4bd804b

Add an illustrative LdpGetOperation.from()

bb68760

Replace Operation.from() factory method with ldp instance factory

239ac2b

Flesh out LdpGetOperation.from()

513d861

Add content-type and -length to common headers

7c96212

Add dispatch logic for LdpRangeOperation

eeee098

Add LdpDeleteOperation.perform() logic

842773c

Update fs-extra to latest

fe9b82f

Implement LdpTarget, refactor LdpHttpHandler accordingly

2595a15

Implement LdpFileStore and LdpFileResource

906e289

Implement deleteResource() & deleteContainer()

00688f7

Pass in fs into LdpFileStore

127dde1

Implement LdpPutOperation logic (and store logic)

976dc87

Add root and multiuser params to host config object

5d7d453

dmitrizagidulin force-pushed the ldp-api branch from 5b4a273 to 5d7d453 Compare October 5, 2018 00:30

Regenerate package-lock.json

2ec6ed2

dmitrizagidulin added 2 commits October 4, 2018 21:01

Refactor/expand SolidHost.parseTargetUrl()

0f7bfc8

Refactor LDP Target, FileResource and Store

ed40c42

jaxoncreed closed this Apr 16, 2019


		const LdpOperation = require('./ldp-operation')

		class LdpApi {

Conversation

dmitrizagidulin commented Jun 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RubenVerborgh Jun 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RubenVerborgh Jun 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmitrizagidulin commented Jun 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

RubenVerborgh Jun 12, 2018 •

edited

Loading

RubenVerborgh Jun 12, 2018 •

edited

Loading