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

SPARQL updates #34

Merged
merged 4 commits into from Jul 23, 2020
Merged

SPARQL updates #34

merged 4 commits into from Jul 23, 2020

Conversation

joachimvh
Copy link
Member

This adds support for SPARQL updates through PATCH requests. Currently only deletes/inserts without variables are supported (will need to look into an internal querying engine potentially for more advanced queries).

The main "special" thing is probably how the input body is handled in a Patch. To prevent multiple parsings, the body parser immediately reads the body and parses it into the algebra. The data field then throws an error if it would still be accessed afterwards (which should never happen since every class that uses a Patch would know of the interface). This was already somewhat discussed with @RubenVerborgh but this can still change of course.

@joachimvh joachimvh added the ☀️ enhancement New feature or request label Jul 22, 2020
Copy link
Contributor

@rubensworks rubensworks left a comment

Choose a reason for hiding this comment

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

Looks great!


public async handle(input: HttpRequest): Promise<SparqlUpdatePatch> {
try {
const sparql = (await arrayifyStream(input)).join('\n');
Copy link
Contributor

Choose a reason for hiding this comment

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

I suspect this stringification will be needed by many body parsers. We may want to consider abstracting this in BodyParser. (not urgent though)

Copy link
Member

Choose a reason for hiding this comment

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

I'm confused by the \n; that surely must be a mistake? They should just be concatenated?

But yes, isn't there a simple arrayToString somewhere? (And if not, let's write it?)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yea no reason for the \n to be there. Probably just seemed nicer in my head at the time to have everything on separate lines.

It is readableToString that is needed here (and the function for that would be that line, can put it in util).

Copy link
Member

Choose a reason for hiding this comment

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

no reason

Even worse, it would mess up chunked input 😉

// Prevent body from being requested again
return {
algebra,
dataType: 'algebra',
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if 'algebra' is descriptive enough for this. Maybe we want 'sparqlAlgebra' or 'sparql-algebra'?

@RubenVerborgh RubenVerborgh self-requested a review July 22, 2020 12:15
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.

Sweet, good stuff! I like how everything is easy to follow.


public async handle(input: HttpRequest): Promise<SparqlUpdatePatch> {
try {
const sparql = (await arrayifyStream(input)).join('\n');
Copy link
Member

Choose a reason for hiding this comment

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

I'm confused by the \n; that surely must be a mistake? They should just be concatenated?

But yes, isn't there a simple arrayToString somewhere? (And if not, let's write it?)

src/storage/PatchStore.ts Outdated Show resolved Hide resolved
* If the original store supports the {@link Patch}, behaviour will be identical,
* otherwise one of the {@link PatchHandler}s supporting the given Patch will be called instead.
*/
export class PatchStore implements ResourceStore {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe PatchingStore or PatcheableStore or something (to avoid the impression that it stores patches)?

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.

None yet

3 participants