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

Support For Swift-HTTP-Types #2758

Open
MahdiBM opened this issue Jun 27, 2024 · 11 comments
Open

Support For Swift-HTTP-Types #2758

MahdiBM opened this issue Jun 27, 2024 · 11 comments

Comments

@MahdiBM
Copy link
Contributor

MahdiBM commented Jun 27, 2024

What are the plans of this package to support swift-http-types?
The best plan I can come up with is to use the Swift 6 package traits to have an option to switch to those types?

@adam-fowler
Copy link
Contributor

I am currently using the conversion channel handlers in https://github.com/apple/swift-nio-extras/tree/main/Sources/NIOHTTPTypesHTTP1. But would love to see conversion straight to swift-http-types instead of going via the types in NIOHTTP1, but that would require implementing the whole of NIOHTTP1 using the new types so upgrade handlers etc are available.

@Joannis
Copy link

Joannis commented Jun 28, 2024

I would love to have swift-http-types supported. We're currently paying conversion costs

@Lukasa
Copy link
Contributor

Lukasa commented Jun 29, 2024

I think at this point we should consider cutting over to the common types.

@MahdiBM
Copy link
Contributor Author

MahdiBM commented Jun 29, 2024

@Lukasa So like in/with a new major version?

@Lukasa
Copy link
Contributor

Lukasa commented Jun 29, 2024

I don’t think that’s necessary. We can deprecate the old APIs and keep them around.

@MahdiBM
Copy link
Contributor Author

MahdiBM commented Jun 29, 2024

Ah ok that was kind of what I was thinking about as well, which made me think about "package traits".
Though normal deprecation of the old APIs and addition of the new APIs could be enough.

@mpapple-swift
Copy link

Bump - any thoughts on this? May i consider putting together a PR for this?

@adam-fowler
Copy link
Contributor

This is more than just writing an encoder, decoder for the HTTP types. You also need to provide the other support channel handlers like pipelining, response header validation, error handling and client and server pipeline upgrades. Of course the encoder/decoder would be the starting point.

@Lukasa
Copy link
Contributor

Lukasa commented Oct 31, 2024

Yeah, Adam is right that this is a bit of a changeover, but if you want to start with the encoder/decoder I think that's a good first step. I think we end up needing the following items changed (@adam-fowler please shout if I've missed anything):

  • New typealiases for the parts. Probably HTTPRequestPart and HTTPResponsePart, as they should now be symmetrical (we can take this opportunity to exercise the cursed IOData type)
  • New typealiases for the decoders.
  • New encoders.
  • Updates to HTTPDecoder to ensure it can handle the new part types
  • Update to HTTPDecoder to ensure it conforms to WriteObservingByteToMessageDecoder in the new contexts.
  • New HTTPServerPipelineHandler
  • New HTTPServerProtocolErrorHandler
  • New server upgrade handlers:
    • HTTPServerUpgradeHandler
    • HTTPServerProtocolUpgrader
    • HTTPServerUpgradeEvents
  • New client upgrade handlers:
    • NIOHTTPClientUpgradeHandler
    • NIOHTTPClientProtocolUpgrader
  • New NIOHTTPRequestHeadersValidator
  • New NIOHTTPResponseHeadersValidator
  • New NIOHTTPClientResponseAggregator (including new NIOHTTPClientResponseFull)
  • New NIOHTTPServerRequestAggregator (including new NIOHTTPServerRequestFull)
  • New typed upgrade handler:
    • NIOTypedHTTPClientProtocolUpgrader
    • NIOTypedHTTPClientUpgradeConfiguration
    • NIOTypedHTTPClientUpgradeHandler
    • NIOTypedHTTPServerProtocolUpgrader
    • NIOTypedHTTPServerUpgradeConfiguration
    • NIOTypedHTTPServerUpgradeHandler
    • NIOUpgradableHTTPClientPipelineConfiguration
    • NIOUpgradableHTTPServerPipelineConfiguration
  • New channel pipeline configurators (though we can get away with fewer, as many of these existed for backwards compatibility)
    • ChannelPipeline
      • addHTTPClientHandlers(position:leftOverBytesStrategy:enableOutboundHeaderValidation:encoderConfiguration:withClientUpgrade:)
      • configureHTTPServerPipeline(position:withPipeliningAssistance:withServerUpgrade:withErrorHandling:withOutboundHeaderValidation:withEncoderConfiguration:)
      • configureUpgradableHTTPServerPipeline(configuration:)
      • configureUpgradableHTTPClientPipeline(configuration:)
    • ChannelPipeline.SynchronousOperations
      • addHTTPClientHandlers(position:leftOverBytesStrategy:enableOutboundHeaderValidation:encoderConfiguration:withClientUpgrade:)
      • configureHTTPServerPipeline(position:withPipeliningAssistance:withServerUpgrade:withErrorHandling:withOutboundHeaderValidation:withEncoderConfiguration:)
      • configureUpgradableHTTPServerPipeline(configuration:)
      • configureUpgradableHTTPClientPipeline(configuration:)
  • Deprecate everything we no longer need, including all types replaced above and most of our other duplicative types.
  • All the existing tests duplicated to cover the new types

This is, I hope it's fairly easy to see, a fairly substantial chunk of work. While we do this I think the various types should remain internal to avoid exposing a partial new API, and to allow us to fix any mistakes in the flow. But we will happily accept PRs from folks who want to start cutting things over.

@adam-fowler
Copy link
Contributor

@Lukasa one thing missed from this, Is what to do with the swift-nio-extras modules that we currently use to support the swift http types.

  • Would the plan be to add new swift http type code in NIOHTTP1? Otherwise need to be careful with module naming to not clash with swift-nio-extra modules.
  • Should HTTP2 conversion handler be moved into swift-nio-http2?
  • Also review what request validation is repeated in both decoder and in swift http type initialisation.

@Lukasa
Copy link
Contributor

Lukasa commented Nov 6, 2024

Would the plan be to add new swift http type code in NIOHTTP1? Otherwise need to be careful with module naming to not clash with swift-nio-extra modules.

My preference would be yes, as we want to keep the access to the decoder and other types.

Should HTTP2 conversion handler be moved into swift-nio-http2?

Yes, but that work is on a different schedule.

Also review what request validation is repeated in both decoder and in swift http type initialisation.

Agreed.

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

No branches or pull requests

5 participants