First I want to see if the CI/CD pipeline fails or not. I have locally issues with running the tests, some unrelated tests fail.

Ok, it seems like my expectation was met and the codechange did not make it fail the ci pipeline.

I guess the unit test passes because it falls back to the json parser. Will provide a unit test asap.

I guess the unit test passes because it falls back to the json parser. Will provide a unit test asap.

Interesting, it is not failed because this.name === this.type most of the time in essence check.
And toString method do the trick.

var a = { toString() { return 'a' } }
'abc'.indexOf(a) // it is actually 0 

One failing test for your changes would be addContentTypeParser('image') should match all the image content-type. But, I doubt it should be added, since developer should provide a correct content-type in first place. This behavior exist only because of the non-breaking change.

Added 3 Unit tests.

Why we would like to support a content type with leading and trailing space?
It should be a side effect of using parser.

Just a reminder, it is breaking change.

I don't think it is breaking

It is a breaking change because here change from .name check to .type check.
Please read the first comment.

Also, if we accept it. It means that #4450 will not be able to land since it will becomes a breaking change again.

ParserListItem.name is passed by user. We may trim it's value if you believe application/json (value given by developer) should be supported.

ParserListItem.type is parsed by library. They are totally two things.

Consider the below case currently supported, but not documented.

test('allow partial content-type', async t => {
  t.plan(1)

  const fastify = Fastify()
  fastify.removeAllContentTypeParsers()
  fastify.addContentTypeParser('json', function (request, body, done) {
    t.pass('should be called')
    done(null, body)
  })

  fastify.post('/', async () => {
    return 'ok'
  })

  await fastify.inject({
    method: 'POST',
    path: '/',
    headers: {
      'content-type': 'application/json; foo=bar; charset=utf8'
    },
    body: ''
  })
})

I tested only the spacing cases, not the fastify.addContentTypeParser('json' 👍🏽

I doubt that that works, because content-type parser will filter out media-types without / in it as invalid.

I doubt that that works, because content-type parser will filter out media-types without / in it as invalid.

Don't mix-up supported content-type and incoming content-type.
Your change affect the supported content-type (which is given by the developer and doesn't require to be a fully specified content-type.), the current state which is allowed by checking the name.
Incoming content-type never affected which require to be a valid content-type.

I still see no point on support application/json as input in addContentTypeParser. Most of the time, it should be a typo specified in code or they really mean it should be a space before or after.

hmm. it seems to me, that the implementation in fastify/multipart is wrong in the first place. It should be multipart/form-data and not multipart.

It should be multipart/form-data and not multipart.

Not really, all the multipart/* is under the same message structure. So, it is a correct assumption to use multipart only.

https://www.rfc-editor.org/rfc/rfc2046#section-5.1.1

This section defines a common syntax for subtypes of "multipart". All subtypes of "multipart" must use this syntax.

Hmm, seems like it blocked for other multipart.
https://github.com/fastify/busboy/blob/master/lib/types/multipart.js#L24

Well, should we target the next branch for this PR?

@climba03003 @Uzlopak could you reach an agreement on the couple of PRs open on this? I don't understand the conflict and what will break.

@climba03003 @Uzlopak could you reach an agreement on the couple of PRs open on this? I don't understand the conflict and what will break.

You can see the example in this comment.
#4477 (comment)

From what I can tell, this.name = contentType.trim() does the same with the case I mention and the test included in this PR.

Could you send a PR with that test #4477 (comment)? So we know it's supported.

I added the unit test of @climba03003 to this PR. tests pass

Please update the test to the following one. You are actually added a parser that would match all content-type.
When parsing a partial is failed, it return '' empty string for the .type and .indexOf('') !== -1 is always true.

test('allow partial content-type', async t => {
  t.plan(1)

  const fastify = Fastify()
  fastify.removeAllContentTypeParsers()
  fastify.addContentTypeParser('json', function (request, body, done) {
    t.pass('should be called')
    done(null, body)
  })

  fastify.post('/', async () => {
    return 'ok'
  })

  await fastify.inject({
    method: 'POST',
    path: '/',
    headers: {
      'content-type': 'application/json; foo=bar; charset=utf8'
    },
    body: ''
  })

  await fastify.inject({
    method: 'POST',
    path: '/',
    headers: {
      'content-type': 'image/jpeg'
    },
    body: ''
  })
})

What about now?

Look better, but still I believe more problem would cause than a fix when we allow trimming.

Consider the following is allowed but the developer have no idea which one should be matched (Currently, the third one).
Some plugin maybe overriding with an extra space, then the content-type parser is actually replaced without notice.

fastify.addContentTypeParser('   application/json   ', function (request, body, done) {
  done(null, body)
})
fastify.addContentTypeParser('application/json   ', function (request, body, done) {
  done(null, body)
})
fastify.addContentTypeParser('   application/json', function (request, body, done) {
  done(null, body)
})

And which one should be removed? Currently, none of them.

fastify.addContentTypeParser('   application/json   ', function (request, body, done) {
  done(null, body)
})
fastify.addContentTypeParser('application/json   ', function (request, body, done) {
  done(null, body)
})
fastify.addContentTypeParser('   application/json', function (request, body, done) {
  done(null, body)
})
fastify.removeContentTypeParser('application/json')

When you consider <space * 0-n>application/json<space * 0-n> is identical to application/json. Any methods (including, .add, .hasParser, .getParser, .remove, etc ) should treat them the same as well.

@Uzlopak are you still pursuing this?

As we never agreed on how to solve this issue, i can not finalize this PR.

@gurgunday
@climba03003

Is this PR finally obsolete? Of are there still some things we can derive from this? Can we close it?

Is this PR finally obsolete?

Is #4509 answered your problem trying to fix in this PR.
As I remembered it was made for this PR.

Of are there still some things we can derive from this?

The next thing we need to consider is #4477 (comment)

The next thing we need to consider is #4477 (comment)

See #5329, this is no longer possible 🙏

I want to actually look at increasing matches though, right now it seems like a Content-Type header like text/html won't match the added parser (because we now do a startsWith check only), which is why we should be trimming it

See #5329, this is no longer possible 🙏

It partially addressed, you need to do the same to .hasParser and .remove.

I believe it covers by #4509, #5329 and #5372.

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.