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

json middleware does not work on content types with a + #519

Open
jeswr opened this issue Mar 17, 2024 · 9 comments
Open

json middleware does not work on content types with a + #519

jeswr opened this issue Mar 17, 2024 · 9 comments

Comments

@jeswr
Copy link

jeswr commented Mar 17, 2024

The json middleware does not parse results as json if the content type is anything but application/json, I would expect it to also work on content types like application/ld+json. For instance:

import express from 'express';
const app = express();
app.use(express.json());

app.post('/', async (req, res) => {
    console.log("Data posted", await req.body);
});

app.listen(3000, () => {
    console.log('Listening on port 3000');
    fetch('http://localhost:3000', {
        method: 'POST',
        headers: {
            'Content-Type': 'application/json',
        },
        body: JSON.stringify({
            message: 'Hello world',
        }),
    });
});

returns

Listening on port 3000
Data posted { message: 'Hello world' }

On the other hand

import express from 'express';
const app = express();
app.use(express.json());

app.post('/', async (req, res) => {
    console.log("Data posted", await req.body);
});

app.listen(3000, () => {
    console.log('Listening on port 3000');
    fetch('http://localhost:3000', {
        method: 'POST',
        headers: {
            'Content-Type': 'application/ld+json',
        },
        body: JSON.stringify({
            message: 'Hello world',
        }),
    });
});

returns

Listening on port 3000
Data posted {}
@jeswr jeswr added the bug label Mar 17, 2024
@dongCode
Copy link

express.json() just for Content-Type: application/json try this middleware ? like this demo
`import express from 'express';
const app = express();

app.use((req, res, next) => {
if (req.is('application/ld+json')) {
req.body = JSON.parse(req.rawBody);
}
next();
});

app.use(express.json());
`

@jeswr
Copy link
Author

jeswr commented Mar 18, 2024

Thank you for the sample code.

I will leave this issue open, as the main reason I opened this issue is in the interest following the robustness principle / postels law - in particular I would expect that any server which can handle application/json to also be able to handle application/ld+json or any other media type with the +json suffix as "Suffix is an augmentation to the media type definition to additionally specify the underlying structure of that media type, allowing for generic processing based on that structure and independent of the exact type's particular semantics" (https://en.wikipedia.org/wiki/Media_type).

@wesleytodd wesleytodd transferred this issue from expressjs/express Mar 18, 2024
@wesleytodd
Copy link
Member

Moved this to the correct repo.

How would you expect to see the req.body with ls+json? I would expect an array where each line is an item parsed as json individually. I feel personally like this would be a new middleware exported by this package if we chose to add support? Not sure I have ever seen anyone use ldjson as input to an http api before, so maybe would be good to give a description of your use case as well?

@jonchurch
Copy link
Member

jonchurch commented Mar 19, 2024

Folks can do this today but have to configure it like this

// docs https://expressjs.com/en/api.html#express.json
app.use(express.json({type: ['application/*+json', 'application/json']}))

Unfortunately "application/*json doesn't work. That is a type-is limitation. I agree that we should have a way to allow parsing of all json subtypes.

If you receive input with +json media type it is understood to be safe to parse as a JSON doc because it must be one. NDJSON cannot be parsed as a single JSON doc so it has a distinct mimetype application/x-ndjson, for example.

I am open to this being the default behavior. Im sure 99% of people are just using application/json, so it's a small gain which wouldn't be worth it for correctness alone if it had some downside. Im not aware of any real ones though except additional load/risk parsing payloads you weren't expecting.

They should still be JSON though and you'd have to go out of your way to enable parsing a valid json doc if you did want to currently.

lil repro:

const express = require('express')
const bodyParser = require('body-parser')

const app = express()

// app.use(express.json({type: "*/*+json"}))
app.use(express.json({type: ['application/*+json', 'application/json']}))

app.post('/', async (req, res) => {
    console.log("Data posted", await req.body);
});

app.listen(3000, () => {
    console.log('Listening on port 3000');
    fetch('http://localhost:3000', {
        method: 'POST',
        headers: {
            'Content-Type': 'application/json',
        },
        body: JSON.stringify({
            message: 'application/json',
        }),
    });
    fetch('http://localhost:3000', {
        method: 'POST',
        headers: {
            'Content-Type': 'application/ld+json',
        },
        body: JSON.stringify({
            message: 'application/ld+json',
        }),
    });
});

@wesleytodd
Copy link
Member

I am open to this being the default behavior

I don't think this should be a default behavior for .json. I don't think api's accepting ld-json are common (elastic bulk updates is the only one I can think of) and I think it is pretty acceptable to have to turn it on. If a client started sending it and my server started crashing because I expected always an object but .json started populating .body with an array I would be pretty miffed.

@jonchurch
Copy link
Member

jonchurch commented Mar 19, 2024

I agree Id be miffed if valid json crashed my server and that it is very unlikely for someone to use one of these for input. We can shelve the idea of it being a default for now, but...

ldjson isnt necessarily an array. But an array is still valid JSON so thats an API implementation detail you can run into regardless of your specific json sub mimetype.

Lets make sure we are talking about the same thing. application/ld+json is Linked Data JSON, its still a valid JSON blob, or potentially a top level array of ldjson objects.

This is valid ldjson, but the fact it is in an array is not inherently part of what makes it ldjson. It can also be a single top level object, same as any JSON blob

[
  {
    "@context": "https://schema.org",
    "@type": "Person",
    "name": "Jane Doe",
    "jobTitle": "Software Engineer",
    "affiliation": "Example Corporation",
    "email": "[email protected]"
  },
  {
    "@context": "https://schema.org",
    "@type": "Person",
    "name": "John Smith",
    "jobTitle": "Graphic Designer",
    "affiliation": "Creative Design Inc.",
    "email": "[email protected]"
  }
]

But the toplevel array possibility is universal to all valid JSON so far as I know, that possibility doesnt relate to ldjson.

I agree it could be confusing, but I am still looking for an actual downside. It is equally confusing that we dont parse valid JSON objects

OData spec looks a lot like ld+json, OData just doesnt use anything but application/json as mime when the payload is json.

@wesleytodd
Copy link
Member

Lets make sure we are talking about the same thing.

I think I just accidentally wrote ld and not nd. Anyway, can't keep up with this convo right now I just wanted to drop a note that doing this by default is something we should avoid if we can imo.

@jonchurch
Copy link
Member

Heard. Ndjson has its own mimetype and would not be impacted by the suggested change.

application/x-ndjson

Mime has the + subclasses to deal with OP's topic specifically.

Any +json should be fully parseable by JSON.parse

application/x-ndjson would not be matched by something like *+json in the proposed changes.

@jeswr
Copy link
Author

jeswr commented Mar 27, 2024

+1 on @jonchurch's points

How would you expect to see the req.body with ls+json? I feel personally like this would be a new middleware exported by this package if we chose to add support?

Just the result of calling JSON.parse on the body, no different to the current behavior of the handler on the application/json content type; since "Any +json should be fully parseable by JSON.parse"

Not sure I have ever seen anyone use ldjson as input to an http api before, so maybe would be good to give a description of your use case as well?

I'm currently working on a project which involves conversational Web agents that are posting RDF to one another, granted I'm parsing the body as a dataset most of the time using @rdfjs/express-handler.

This would come up in production settings when implementing specs like Verifiable Credentials where there was a division between spec writers on whether to use plain JSON or RDF representations for the VCs. The compromise was that framed JSON-LD would be used. The specification uses POSTing of framed JSON-LD in order to issue credentials.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants