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

Improve how ShortUrl relations are resolved (domains and tags) #862

Closed
acelaya opened this issue Oct 22, 2020 · 0 comments · Fixed by #995
Closed

Improve how ShortUrl relations are resolved (domains and tags) #862

acelaya opened this issue Oct 22, 2020 · 0 comments · Fixed by #995
Milestone

Comments

@acelaya
Copy link
Member

acelaya commented Oct 22, 2020

Currently, when creating a ShortUrl, there are two entities that could need to be created or fetched as a side effect, Tag and Domain.

These are always provided as strings that reference to a unique index for their respective entities, which result in the tag/domain being created if it does not exist, or fetched otherwise, resulting in a relationship with ShortUrl in both cases.

This is currently handled differently for tags and domains.

For domains

The ShortUrl expects an object implementing DomainResolverInterface to be optionally provided. If it is not provided, it falls back to the basic implementation, SimpleDomainResolver, which just returns a new Domain instance always.

However, at runtime, a PersistenceDomainResolver is always provided, which depends on the EntityManager, and therefore, it is capable of persisitng/fetching actual domains from persistence layer.

For tags

Services that need to create the list of tags for a ShortUrl, use the TagManagerTrait, which exposes a method to create/fetch tags.

These tags are later set to the ShortUrl via setter.

Proposal

These are the problems we need to resolve:

  • Lack of consistency between how these entities are handled.
  • Usage of a trait in the second case.
  • Requirement of a setter in the second case.

The way in which this problem is handled for domains is better than the other one, but having to pass "resolvers" for each one of them makes it a bit cumbersome.

One option is to have a single "resolver" that can handle all entities and exposes public methods to persist/fetch any kinds of entity.

We would still keep a "simple" implementation to which the logic falls back (for unit tests and such).


Edit 2021-01-04

As part of #882, the DomainResolverInterface interface is now ShortUrlRelationResolverInterface, which means we can add the logic for tags here.


Edit 2021-01-31

As part of this, the PATCH /short-urls/{shortCode} endpoint can gain support to edit the short URL tags.

That should effectively deprecate the PUT /short-urls/{shortCode}/tags endpoint.

It's also interesting if the PATCH endpoint returns the serialized short URL instead of an empty response.

@acelaya acelaya added this to the 2.5.0 milestone Oct 22, 2020
@acelaya acelaya changed the title Improve how ShortUrl relations are resolved (domains and tags) Improve how ShortUrl relations are resolved (domains, tags and API keys) Nov 7, 2020
@acelaya acelaya modified the milestones: 2.5.0, 2.6.0 Jan 2, 2021
@acelaya acelaya changed the title Improve how ShortUrl relations are resolved (domains, tags and API keys) Improve how ShortUrl relations are resolved (domains and tags) Jan 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant