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

feat: Add feature to parse multiple set-cookies #688

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

myConsciousness
Copy link

Hi amazing developers,

Perhaps this is already on the agenda before, but the http library combines multiple set-cookies in the response header into a comma-separated list that is set in the headers field. This is a rather complicated specification and not one that developers can easily handle.

Therefore, I have added the feature to parse set-cookies contained in headers to make it easier to handle multiple set-cookies. This modification is not disruptive and the headers field will continue to function as before.

However, in the future you need to reintegrate cookies field when you make the headers field an object, as was written in the BaseResponse TODO.

Also it was necessary to add universal_io to the dependencies to use Cookie object.

Thank you.

@devoncarew
Copy link
Member

Thanks for the PR! Some thoughts:

  • this is a highly used package, so we generally want to be very conservative about adding a new package dependency. You're adding package:universal_io - I assume to get the definition of the Cookie class? We'll likely have to remove the dep. It looks like package:universal_io exports the dart:io Cookie class in a dart:io context, and defines it's own class in other contexts. I don't know if that makes sense to do in this library, but fyi
  • adding @natebosch for review (and @brianquinlan for fyi, in case he has comments on the PR)

@myConsciousness
Copy link
Author

myConsciousness commented Apr 26, 2022

Thanks for your feedback @devoncarew !

This is exactly what I was thinking too.

I used to get an error in static analysis when I uploaded a package I was developing that used dart:io to Pub.dev, and this is the reason for adding universal_io but now it seems I misunderstood the cause of that error.

And now I have scrutinized the universal_io implementation and found no reason to use universal_io instead of dart:io in this fix. I'm going to refactor my pull request.

Thank you.

@@ -2,6 +2,8 @@
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.

import 'dart:io';
Copy link
Member

Choose a reason for hiding this comment

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

We can't import dart:io from this library. universal_io is intending to get around that problem, but we don't want to take a dependency on that library either. If we wanted to have this functionality in this library we'd have to reimplement an interface for Cookie.

This feels like the type of thing that should be added on in a side package (which could more easily have a dependency on universal_io)

extension Cookies on Response {
  List<Cookie> get cookies {
    // ...
  }
}

I suppose the nice thing about implementing it here is being able to cache it, but using an expando is probably a fine alternative if caching is important. Probably you'll only read .cookies once anyway.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for your review @natebosch !

I found several issues have been raised regarding this case of multiple set-cookies, and I feel that a fundamental solution is needed. At the same time, I also understood the difficulties related to dart:io. It's quite a deep-rooted problem.

It's possible to add a Cookie interface and implementation, but is there a reluctance to add a Cookie object as a development policy for the http library?

If we can implement Cookie interface in http library, I thought the following example was a very good solution.

extension Cookies on Response {
  List<Cookie> get cookies {
    // ...
  }
}

@vaind
Copy link

vaind commented Nov 2, 2022

FYI this is related to #24 which proposes a different approach

for (final cookie in response.cookies) {
expect(
cookie.name,
anyOf([
Copy link

Choose a reason for hiding this comment

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

this makes the test pass if response.cookies is always just a single cookie, e.g. the first one, repeated 6 times. This isn't so unimaginable in case of an error in the parsing code.

@XNORGATE
Copy link

XNORGATE commented Apr 24, 2023

up ,there's still an issue about parsing multiple set-cookies now , all set-cookies will be covered by last one
hope this can be merged asap

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

Successfully merging this pull request may close these issues.

None yet

5 participants