Fix TriG documents not being PUT-able #773

rubensworks · 2021-06-11T11:37:19Z

A temporary fix until a more generic solution is implemented (#768).

src/util/ContentTypes.ts

src/storage/mapping/ExtensionBasedMapper.ts

joachimvh · 2021-06-11T12:33:17Z

The reason the integration tests failed is because it also needs to be added in https://github.com/solid/community-server/blob/main/templates/config/filesystem.json .

The templates for dynamic pods don't fully make use of the new configuration setup yet so there's some duplication there.

rubensworks · 2021-06-11T14:16:09Z

Amended the commit with all suggested changes.

RubenVerborgh

Great, let's get the duplication out though. Can be a new issue as far as I'm concerned.

RubenVerborgh · 2021-06-13T19:06:16Z

src/storage/mapping/ExtensionBasedMapper.ts

+  public constructor(
+    base: string,
+    rootFilepath: string,
+    overrideTypes: Record<string, string> = { acl: TEXT_TURTLE, meta: TEXT_TURTLE, trig: APPLICATION_TRIG },


Let's drop the default here—or leave it as it was.

The rationale is the following.

This class is expected to perform a "perfect" mapping from extensions to MIME types, with the (sole) exception of Solid-specific MIME types. So acl and meta are special in the sense that they do not exist outside of the Solid world. In contract, trig exists outside of Solid; it's basically a "bug" in mime-types (can we PR?).

According to the docs it uses the db here and they accept PRs https://github.com/jshttp/mime-db

application/trig is a content-type they recognize as existing, but it does not have an official extension associated with it in the db is the issue, as it does not have any in the official databases this library makes use of.

I maybe wonder whether we want to rename overrideTypes into customTypes perhaps.

jshttp/mime-db#238

src/storage/mapping/ExtensionBasedMapper.ts

RubenVerborgh · 2021-06-13T19:10:15Z

config/util/identifiers/subdomain.json

+        },
+        {
+          "SubdomainExtensionBasedMapper:_overrideTypes_key": "trig",
+          "SubdomainExtensionBasedMapper:_overrideTypes_value": "application/trig"


Perhaps comment that this is a bug/omission in mime-types? (see below)

RubenVerborgh · 2021-06-13T19:12:13Z

src/storage/mapping/SubdomainExtensionBasedMapper.ts

@@ -36,7 +36,7 @@ export class SubdomainExtensionBasedMapper extends ExtensionBasedMapper {
  private readonly baseParts: { scheme: string; rest: string };

  public constructor(base: string, rootFilepath: string, baseSubdomain = 'www',
-    overrideTypes = { acl: TEXT_TURTLE, meta: TEXT_TURTLE }) {
+    overrideTypes: Record<string, string> = { acl: TEXT_TURTLE, meta: TEXT_TURTLE, trig: APPLICATION_TRIG }) {


I definitely do not like this duplication here. Let's export this object from ./../util/ContentTypes?
Then I also have no objection for using it as the default in ExtensionBasedMapper above.

We could also just remove the default values from the constructor actually since they're in the config anyway.

Yeah, but the problem is that these values need to be encoded multiple times in config.

RubenVerborgh · 2021-06-13T19:13:21Z

config/util/identifiers/suffix.json

+          "ExtensionBasedMapper:_overrideTypes_key": "trig",
+          "ExtensionBasedMapper:_overrideTypes_value": "application/trig"
+        }
+      ]


Here's me wishing that the object could be reused across subdomain.json and suffix.json, but I understand why that is currently not possible. Future idea for Components.js to have generic key/value props?

rubensworks · 2021-06-22T15:35:59Z

So I guess this PR can just be replaced with a version bump of mime-db once the change in jshttp/mime-db#238 is included in a next release.
If so, let's put this once on hold until then.

RubenVerborgh · 2021-07-05T08:36:26Z

I have pinged jshttp/mime-db#238

RubenVerborgh · 2021-07-05T08:37:30Z

this PR can just be replaced

I do like the genericness of the solution though; would still keep it and its tests, just not the .trig fix.

RubenVerborgh · 2021-07-22T11:22:52Z

Might need the override system in any case; .ts is not TypeScript at the moment, for instance.

RubenVerborgh · 2021-07-27T07:53:40Z

A new version of the MIME library has been released, so we can adjust this PR.

Note that I'd still like to keep the option of having overrides.

RubenVerborgh · 2021-07-27T07:55:23Z

@rubensworks But feel free to pass this on to anyone else; just so we have a champion to bring this to v1.0.0.

rubensworks · 2021-07-27T08:00:52Z

Since mime-db is updated, the scope of this PR doesn't really align with my use case anymore, so not sure when I'll be able to look into this.
If it's urgent for the 1.0.0 release, perhaps @joachimvh is willing to take this up?

joachimvh · 2021-07-27T08:03:29Z

If it's urgent for the 1.0.0 release, perhaps @joachimvh is willing to take this up?

Sure

joachimvh · 2021-07-27T12:45:58Z

A new version of the MIME library has been released, so we can adjust this PR.

The mime-types library also has to be updated to use the updated version of mime-db.

Or we can use the mime-db dependency directly and implement the wanted behaviour ourselves.

RubenVerborgh · 2021-07-27T12:59:18Z

The mime-types library also has to be updated to use the updated version of mime-db.

Just a package-lock upgrade is fine with me.

joachimvh · 2021-07-27T14:48:34Z

I have updated the commit. Now it is no longer necessary to set the defaults custom types in the config.

Trig is still in there since mime-types hardcodes its dependency in the package.json: https://github.com/jshttp/mime-types/blob/2.1.31/package.json#L17 . We can leave this open until that gets updated, is only a small change afterwards.

Using both mime functions for consistency. Not using the maps since mime.extensions stores arrays instead of best matches.

RubenVerborgh · 2021-07-28T11:28:09Z

src/util/ContentTypes.ts

@@ -11,3 +12,6 @@ export const TEXT_TURTLE = 'text/turtle';
 export const INTERNAL_ALL = 'internal/*';
 export const INTERNAL_QUADS = 'internal/quads';
 export const INTERNAL_ERROR = 'internal/error';
+
+// Trig can be removed once the mime-types library is updated with the latest mime-db version
+export const DEFAULT_CUSTOM_TYPES = { acl: TEXT_TURTLE, meta: TEXT_TURTLE, trig: APPLICATION_TRIG };


Perhaps separate by newlines for easier access?

rubensworks · 2021-07-30T17:11:37Z

@joachimvh FYI, It looks like mime-types has been updated to 2.1.32, which includes the new mime-db.

Edit: created a PR for it in #882

rubensworks requested a review from joachimvh June 11, 2021 11:37

rubensworks force-pushed the fix/put-trig branch from d7aa118 to da8f5a0 Compare June 11, 2021 11:47

RubenVerborgh self-requested a review June 11, 2021 11:59

RubenVerborgh self-assigned this Jun 11, 2021

joachimvh requested changes Jun 11, 2021

View reviewed changes

src/util/ContentTypes.ts Show resolved Hide resolved

src/storage/mapping/ExtensionBasedMapper.ts Outdated Show resolved Hide resolved

src/storage/mapping/ExtensionBasedMapper.ts Outdated Show resolved Hide resolved

rubensworks force-pushed the fix/put-trig branch from da8f5a0 to e494d31 Compare June 11, 2021 14:15

RubenVerborgh reviewed Jun 13, 2021

View reviewed changes

RubenVerborgh removed their assignment Jun 13, 2021

RubenVerborgh added the ⌛ has dependency label Jul 5, 2021

RubenVerborgh marked this pull request as draft July 5, 2021 08:37

RubenVerborgh added this to the v1.0.0 milestone Jul 5, 2021

RubenVerborgh removed the ⌛ has dependency label Jul 27, 2021

RubenVerborgh assigned rubensworks Jul 27, 2021

joachimvh assigned joachimvh and unassigned rubensworks Jul 27, 2021

joachimvh force-pushed the fix/put-trig branch 2 times, most recently from 4424375 to 6589c13 Compare July 27, 2021 14:47

joachimvh marked this pull request as ready for review July 27, 2021 14:48

joachimvh requested a review from RubenVerborgh July 27, 2021 14:48

RubenVerborgh approved these changes Jul 28, 2021

View reviewed changes

feat: Update ExtensionBasedMapper custom types

f01fc45

joachimvh force-pushed the fix/put-trig branch from 6589c13 to f01fc45 Compare July 28, 2021 11:36

joachimvh approved these changes Jul 28, 2021

View reviewed changes

joachimvh merged commit 3f8f822 into main Jul 28, 2021

joachimvh deleted the fix/put-trig branch July 28, 2021 11:56

joachimvh mentioned this pull request Jul 28, 2021

Cannot POST file with unknown content type to filesystem-based backend #871

Closed

rubensworks mentioned this pull request Jul 30, 2021

chore: bump mime-types with support for TriG #882

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix TriG documents not being PUT-able #773

Fix TriG documents not being PUT-able #773

rubensworks commented Jun 11, 2021

joachimvh commented Jun 11, 2021

rubensworks commented Jun 11, 2021

RubenVerborgh left a comment

RubenVerborgh Jun 13, 2021

joachimvh Jun 14, 2021

RubenVerborgh Jun 14, 2021

RubenVerborgh Jun 14, 2021

RubenVerborgh Jun 13, 2021

RubenVerborgh Jun 13, 2021

joachimvh Jun 14, 2021

RubenVerborgh Jun 14, 2021

RubenVerborgh Jun 13, 2021

rubensworks commented Jun 22, 2021

RubenVerborgh commented Jul 5, 2021

RubenVerborgh commented Jul 5, 2021

RubenVerborgh commented Jul 22, 2021

RubenVerborgh commented Jul 27, 2021

RubenVerborgh commented Jul 27, 2021

rubensworks commented Jul 27, 2021

joachimvh commented Jul 27, 2021

joachimvh commented Jul 27, 2021

RubenVerborgh commented Jul 27, 2021

joachimvh commented Jul 27, 2021

RubenVerborgh Jul 28, 2021

rubensworks commented Jul 30, 2021 •

edited

Loading

Fix TriG documents not being PUT-able #773

Fix TriG documents not being PUT-able #773

Conversation

rubensworks commented Jun 11, 2021

joachimvh commented Jun 11, 2021

rubensworks commented Jun 11, 2021

RubenVerborgh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rubensworks commented Jun 22, 2021

RubenVerborgh commented Jul 5, 2021

RubenVerborgh commented Jul 5, 2021

RubenVerborgh commented Jul 22, 2021

RubenVerborgh commented Jul 27, 2021

RubenVerborgh commented Jul 27, 2021

rubensworks commented Jul 27, 2021

joachimvh commented Jul 27, 2021

joachimvh commented Jul 27, 2021

RubenVerborgh commented Jul 27, 2021

joachimvh commented Jul 27, 2021

Choose a reason for hiding this comment

rubensworks commented Jul 30, 2021 • edited Loading

rubensworks commented Jul 30, 2021 •

edited

Loading