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

Add explicit unique ResourceTag-like keys #107

Open
zainab-ali opened this issue Nov 7, 2024 · 4 comments
Open

Add explicit unique ResourceTag-like keys #107

zainab-ali opened this issue Nov 7, 2024 · 4 comments

Comments

@zainab-ali
Copy link
Contributor

This issue was copied over from: disneystreaming/weaver-test#266
It was opened by: nigredo-tori


This is inspired by the vault library.

At the moment global resources are indexed by ResourceTags and labels. This has its issues:

  1. The default ResourceTag implementation derives its identity from ClassTag. This leads to a possibility of collisions, as stated in the documentation. The suggested workarounds are monomorphic wrappers or custom ResourceTag instances.
  2. Labels mitigate the collision problem somewhat (or, rather, leave it up to discipline). But the labels are not tied to resource types, so it's easy to mix up which labels correspond to which resource.

Instead, we can create objects that are guaranteed to be unique, and which are associated with a specific type. Consider something like this:

// default `equals` - different objects are never equal.
final private class UniqueResourceTag[A](
  val description: String
) extends ResourceTag[A] {
  def cast(obj: Any): Option[A] = Some(obj.asInstanceOf[A])
}
def unsafeCreateUniqueTag[A](description: String): ResourceTag[A] =
  new UniqueResourceTag[A](description)

This can be used as follows:

// A "constant" somewhere in a common object
val fooTag = unsafeCreateUniqueTag[Foo]("")
// Inside a suite
global.getR()(fooTag)

The current API is obviously not perfect for this, but the general idea should be clear enough. Note that there's no way for fooTag to collide with anything else (assuming no broken equals implentations), and there's no way to mistake the type of the resource it points to.

@zainab-ali
Copy link
Contributor Author

This comment was copied over from: disneystreaming/weaver-test#266 (comment)
It was written by: Baccata


Hello, and thank you for raising an issue

But the labels are not tied to resource types

As a matter of fact they are : ie the ResourceMap is keyed by ResourceTag + Option[String].

This leads to a possibility of collisions, as stated in the documentation.

Yes, and for this very reason we are protecting against it via forcing implicit ambiguity when collisions are possible (ie compile error rather than surprising runtime behaviour), and we've made the conscious decision to leave the API flexible enough so that users can bring in their own patterns (exactly like what you suggest if they want to use that).

The reason why I don't want to support your suggestion out of the box is that, unlike vault's examples, it's impossible to pass the values around logically, from the creation of a key, to its use. So such instances have to be instantiated globally, and for it to work, they have to be assigned to a val and not a def (otherwise you get a new instance on every callsite and referential equality can't kick-in). So as a library author/maintainer, I'd have to protect against assigning these to def via some macro (cause I can guarantee that some users will f***-up), and suddenly the maintenance burden increases.

So I'm more inclined to let this problem be solved by users when they encounter it, than introducing a new construct which is inherently risky without macros, and I don't really want to have to maintain more macros than what we already have. The existing solution might not be perfect, but at least mis-using classtag-based resourceTags lead to compile errors.

However, I'd be willing make a version of global.getR (and its putR dual) that'd take the tag explicitly rather than implicitly, so that solving this in userland wouldn't look as weird.

@zainab-ali
Copy link
Contributor Author

This comment was copied over from: disneystreaming/weaver-test#266 (comment)
It was written by: nigredo-tori


As a matter of fact they are : ie the ResourceMap is keyed by ResourceTag + Option[String].

I guess I didn't phrase it right. Let's say I have putR[Foo](foo, "something".some) as my global setup, and getOrFailR[Foo]("something".some) in my test suites. Now let's say I have decided that instead of Foo I want to share a Bar. I change the first part to putR[Bar](bar, "something".some). Now all the getOrFailR parts compile fine, but fail at runtime!

Instead, if I had a single val somethingTag: ResourceTag[Foo] somewhere, and changed it to ResourceTag[Bar], then I would just have to follow the compiler errors.

So as a library author/maintainer, I'd have to protect against assigning these to def via some macro (cause I can guarantee that some users will f***-up), and suddenly the maintenance burden increases.

Fair point. I can't say for sure whether guarding this with macros would be warranted, but if you think it would be, then this becomes a non-trivial undertaking. Since I can rely on my coworkers being principled enough, I'm fine with keeping the "dangerous" version in our in-house "bits and bobs" library. 😄

@zainab-ali
Copy link
Contributor Author

This comment was copied over from: disneystreaming/weaver-test#266 (comment)
It was written by: Baccata


Since I can rely on my coworkers being principled enough, I'm fine with keeping the "dangerous" version in our in-house "bits and bobs" library.

👍 that sounds like a good work environment 😄

Instead, if I had a single val somethingTag: ResourceTag[Foo] somewhere, and changed it to ResourceTag[Bar], then I would just have to follow the compiler errors.

So would a method that takes tags explicitly be a satisfying middle-ground ?

@zainab-ali
Copy link
Contributor Author

This comment was copied over from: disneystreaming/weaver-test#266 (comment)
It was written by: nigredo-tori


So would a method that takes tags explicitly be a satisfying middle-ground ?

Yes.

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

1 participant