feat: get reply cookie

[gurgunday]

I'm open to feedback if there's an alternative way to do this

Would close #269

[Uzlopak]

Actually if we read #269 than the use case is that multiple cookie plugins can cause trouble.

So I assume, that we maybe have to change the Symbols to use Symbol.for, so that we use one kReplySetCookies symbol through all fastifyCookie instances.

Then we should consider checking if some other plugin set already setCookie headers. Like manually changed without fastify cookie plugin. If so, parse the set-cookie header and add it to the Map of cookies.

[gurgunday]

Oh, that sounds great

We'd need to restructure the plugin, but it could work much better

[gurgunday]

Then we should consider checking if some other plugin set already setCookie headers. Like manually changed without fastify cookie plugin. If so, parse the set-cookie header and add it to the Map of cookies.

You are right, that parsing could become expensive though

[Uzlopak]

Sure, but you could store a hash of the processed externally set set-cookie value in a shared symbol. I assume that hashing the value of set-cookie is faster than actually parsing it.

[gurgunday]

I checked it out and the reason we don't do that is because cookie can only be registered once

In any case, it should already be acting like Symbol.for anyway, since it's defined at the outer-scope

[climba03003]

So I assume, that we maybe have to change the Symbols to use Symbol.for, so that we use one kReplySetCookies symbol through all fastifyCookie instances.

Symbol defined in outer scope (file level) is consistence between each register.
It is how the file cache works.

[gurgunday]

I'm not sure if getCookie should always return an array or just the cookie object if there is only one cookie set for that cookieName (which is the case 99% of the time)

[Uzlopak]

I think it should be an array. Maybe a frozen Array, so that nobody thinks, they can directly modfiy it.

[gurgunday]

Maybe a frozen Array, so that nobody thinks, they can directly modfiy it.

Would that slow it down? I mean, even if that array is modified, it won't have side effects

[Uzlopak]

If size is 0, we should get null anyway?

[Uzlopak]

Oh. We can set multiple cookies? Thought we disalllow this?

[gurgunday]

During the same request, we might set a cookie with the same name for the domains blog.example.com and example.com, or for the paths /foo and /bar

So we do allow multiple cookies that have the same name if their path or domain attributes are different

getCookie doesn't do an advanced search, so it just checks if the names match, and returns the found cookies

[jsumners]

If this method is the way forward, it should always return an array. The client can determine if cookies exists via foundCookies.length.

[Uzlopak]

Imho the performance loss is acceptable. The use of the getCookie function seems like bad idea in the first place. Like somebody doesnt trust his belt and wears additional suspenders. So if somebody uses getCookie than freeze it.

Maybe returning an array is bad anyway, if we have one element? I am now a little bit confused.

Maybe @mcollina or @climba03003 have better ideas. So lets wait for their response.

[mcollina]

lgtm

[jsumners]

Why introduce a completely different API on reply than exists on request?

[gurgunday]

To be certain, do you mean why not something like reply.cookies.name? If so, because the cookies aren't stored in an Object but a Map, we can't use a simple object, also, we have to do a search so it can't be as simple as a property access. It kinda mirrors setCookie

If you instead mean why I didn't decorate the request, it felt like, since these are the cookies to be sent back to the client, reply was appropriate

[jsumners]

I mean that cookies on the request are accessible from request.cookies['some-cookie']. I think that the API on the reply should be consistent. As for the storage mechanism:

const map = new Map()
map._set = map.set
map.set = function set(name, value) {
	map._set.call(this, name, value)
	Object.defineProperty(map, name, { value })
}

Or several other methods of a custom Map implementation.

also, we have to do a search so it can't be as simple as a property access.

I don't follow.

[gurgunday]

I will take a look at this, setCookie(name, value) could also just set the resulting cookie object as a property of reply.cookies

However, that would make Map redundant if I'm not mistaken and cost us in terms of performance (from Map to Object in dictionary mode)

Could indeed be more elegant

[jsumners]

If this method is the way forward, it should always return an array. The client can determine if cookies exists via foundCookies.length.

[Galienna]

Imho the performance loss is acceptable. The use of the getCookie function seems like bad idea in the first place. Like somebody doesnt trust his belt and wears additional suspenders. So if somebody uses getCookie than freeze it.

In fact it's not about trusting your own belt. I have different endpoints where I call several partners. They may reply with some cookies. I don't really know which ones, I only know I must trust my partners and forward the cookies to my final clients.
So, when I call a partner, I iterate over the cookie list and call reply.setCookie.
Now, for each request, I want to monitor the names of the cookies that will be replied. Inside the preSerialization hook, i wanted to be able to call something like reply.getCookies(), then log the cookies names.
Maybe it's not the best way of doing this, but it's the way I can afford regarding my ecosystem.

On a side note, thank you guys for the PR, you're all really efficient <3

[climba03003]

I would say the it is easier to implement something similar to deno

class Cookie {
  constructor(/* cookies attributes here*/) {}
  
  toSetCookie() {
    return // string here
  }
}

const kReplyCookiesStore = Symbol('kReplyCookiesStore')

fastify.decorateReply(kReplyCookiesStore, null)
fastify.decorateReply('cookies', {
  get() {
    return new Proxy(this[kReplyCookiesStore], {
      get(target, prop, receiver) {
        const cookie = target[prop];
        return cookie.value 
      },
      set(obj, prop, value) {
        if(value instanceof Cookie) {
          obj[prop] = value
        } else if (obj[prop]) {
          obj[prop].value = value
        } else {
          obj[prop] = new Cookie(prop, value, defaultOption)
        }
        return true
      }
    })
  }
})

It would be simple to concat the cookies and stringify them by loop the store and call toSetCookie

[mcollina]

So, when I call a partner, I iterate over the cookie list and call reply.setCookie.
Now, for each request, I want to monitor the names of the cookies that will be replied. Inside the preSerialization hook, i wanted to be able to call something like reply.getCookies(), then log the cookies names.

This does look as something user-level. I would recommend you to define your reply decorator, and call reply.setCookie during onSend.

[gurgunday]

This does look as something user-level. I would recommend you to define your reply decorator, and call reply.setCookie during onSend.

Honestly, this to me is the proper Fastify solution to the problem

[gurgunday]

I would say the it is easier to implement something similar to deno

class Cookie {
  constructor(/* cookies attributes here*/) {}
  
  toSetCookie() {
    return // string here
  }
}

const kReplyCookiesStore = Symbol('kReplyCookiesStore')

fastify.decorateReply(kReplyCookiesStore, null)
fastify.decorateReply('cookies', {
  get() {
    return new Proxy(this[kReplyCookiesStore], {
      get(target, prop, receiver) {
        const cookie = target[prop];
        return cookie.value 
      },
      set(obj, prop, value) {
        if(value instanceof Cookie) {
          obj[prop] = value
        } else if (obj[prop]) {
          obj[prop].value = value
        } else {
          obj[prop] = new Cookie(prop, value, defaultOption)
        }
        return true
      }
    })
  }
})

It would be simple to concat the cookies and stringify them by loop the store and call toSetCookie

Thank you for the suggestion, it looks interesting!

I will take a look

[Galienna]

This does look as something user-level. I would recommend you to define your reply decorator, and call reply.setCookie during onSend.

Nope, it's about getting the cookie names and log them. Setting the cookies is done inside the handler :)
From the onSend, reply.getHeader('set-cookie') already returns all the cookies, so it's OK. But from the handler and the preSerialization hook, i have no way to read the reply cookies set with fastify-cookie. That's why i'm interested by à getCookies method

But i still don't understand why fastify-cookie and reply.getHeader('set-cookie') are not in sync. Is it for performance issue ?

[gurgunday]

It is always important to be reminded of why a feature/functionality exists :)

The PR (#237) aimed to prevent duplicate cookies from being sent if during the lifecycle it gets set multiple times, sending duplicate cookies to the client is undefined behavior in the RFC and may cause issues, as a separate plugin dealing exclusively with cookies, managing such scenarios falls within the scope of cookie, I believe.

And it was recommended during that PR to make the cookie object private since it had a custom structure

[gurgunday]

I can't bring myself to finish this in its current form as the solution to what problems this PR attempts to mitigate is to just to store cookies in a decorated object/map and set them once during onSend

Feel free to continue the work and we shall surely review :)