Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a metadata parser #175

Merged
merged 2 commits into from
Oct 9, 2020
Merged

Add a metadata parser #175

merged 2 commits into from
Oct 9, 2020

Conversation

joachimvh
Copy link
Member

Resolves #75 .

Since we now have decent metadata support, we still need a clean way to put the incoming metadata in there. So this is my suggestion for it.

Note that for the MetadataParsers their functionality is similar to AsyncHandlers but I used a new interface since the contracts is slightly different (it's no problem if they can't handle it, and all parsers should be executed regardless of the result of others).

@joachimvh joachimvh added the ☀️ enhancement New feature or request label Oct 6, 2020
Copy link
Member

@RubenVerborgh RubenVerborgh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the handler idea, but

      metadata: await this.metadataHandler.handleSafe(input),

seems repetitive.

Why don't the parsers just not return any metadata?
And then we have one wrapping parser whose sole job it is to add the metadata (so to execute that line)?

@joachimvh
Copy link
Member Author

joachimvh commented Oct 7, 2020

Why don't the parsers just not return any metadata?
And then we have one wrapping parser whose sole job it is to add the metadata (so to execute that line)?

The reason it's currently like this is because BodyParsers return a Representation which needs to contain a RepresentationMetadata. Other options include replacing that line for every BodyParser with metadata: new RepresentationMetadata(), and then passing that metadata object along to the MetadataHandler (is that what you meant with the above statement?). Or running the MetadataHandler first and then passing the resulting metadata to the BodyParsers. Or making metadata optional in the RepresentationMetadata (maybe not this one since several parts of the code assume there is always a content-type so would require several extra checks further down the line).

@RubenVerborgh
Copy link
Member

(After offline discussion:)

Let's have body parsers take metadata as input, to which they can add if they want to, and then return as part of the representation. The metadata is provided by metadata extractors, and can inform body parsing.

Copy link
Member

@RubenVerborgh RubenVerborgh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really good stuff, I like the design. Very elegant an extensible.

Just nits—process as you see fit. No need for me to re-review.

config/presets/ldp/metadata-handler.json Show resolved Hide resolved
src/ldp/http/BasicRequestParser.ts Outdated Show resolved Hide resolved
src/ldp/http/BodyParser.ts Show resolved Hide resolved
src/ldp/http/RawBodyParser.ts Outdated Show resolved Hide resolved
src/ldp/http/RawBodyParser.ts Outdated Show resolved Hide resolved
src/ldp/http/metadata/LinkTypeParser.ts Outdated Show resolved Hide resolved
src/ldp/http/metadata/LinkTypeParser.ts Outdated Show resolved Hide resolved
src/ldp/http/metadata/LinkTypeParser.ts Outdated Show resolved Hide resolved
src/util/HeaderUtil.ts Outdated Show resolved Hide resolved
src/util/HeaderUtil.ts Outdated Show resolved Hide resolved
@joachimvh joachimvh merged commit 31844a4 into master Oct 9, 2020
@joachimvh joachimvh deleted the metadata-parser branch October 9, 2020 10:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
☀️ enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement metadata parser
2 participants