short-d / short

URL shortening service written in Go and React
https://short-d.com
MIT License
869 stars 148 forks source link

Add GraphQL API to update the mutated short links #239

Open andyyaldoo opened 5 years ago

andyyaldoo commented 5 years ago

From #237

oatovar commented 4 years ago

I can take on this issue, and update everyone this Friday regarding the progress made.

oatovar commented 4 years ago

Required Steps

  1. Propose GraphQL API schema changes
  2. Add urlupdater.go and urlupdater_test.go to the usecases url package.
  3. Add approved changes to GraphQL API schema and resolvers.
  4. Update wire.go for dependency injection.
  5. Integrate the update shortlink api with the frontend.
  6. Toogle to true within the feature toggle
oatovar commented 4 years ago

GraphQL Schema

type AuthMutation {
    createURL(url: URLInput!, isPublic: Boolean!): URL
    updateURL(alias: String!, url: URLInput!): URL
    deleteURL(alias: String!)
}
magicoder10 commented 4 years ago

@oatovar Looks great to me!

For your information, we may consider combining all operations on urls to a single place. Maybe still keep the separate interfaces. Not sure what to do yet. The goal is to reduce unnecessary complexities.

oatovar commented 4 years ago

@byliuyang Definitely understandable, making this flexible enough for future additions takes time.

oatovar commented 4 years ago

@byliuyang Should the user have the ability to mutate the short alias as well as the long link?

magicoder10 commented 4 years ago

@oatovar I think so.

oatovar commented 4 years ago

So while testing the changes I found that reusing URLInput as previously suggested doesn't allow for updates to only update the alias. The OriginalURL parameter is always required when updating a short link. I'll be posting my new suggested schema changes to gracefully handle updates to all portions of a short link (user url).

magicoder10 commented 4 years ago

@oatovar I checked the schema. It forces us to provide original URL. Looking forwards for your proposal.

oatovar commented 4 years ago

So for the schema I believe it might be worth the tradeoff of making a new input type that is used by updateURL like so.

input URLUpdateInput {
    originalURL: String
    customAlias: String
    expireAt: Time
}

type AuthMutation {
    createURL(url: URLInput!, isPublic: Boolean!): URL
    updateURL(oldAlias: String!, url: URLUpdateInput!): URL
    createChange(change: ChangeInput!): Change!
    viewChangeLog: Time!
}

While it doesn't reuse an existing type, this does remove the need to include the OriginalURL in every updateURL mutating request.

magicoder10 commented 4 years ago

@oatovar It's seems that Facebook officially used only one input type: https://graphql.org/graphql-js/mutations-and-input-types/

oatovar commented 4 years ago

@byliuyang Thank you for the referenced example! In the example, the application doesn't verify if the content and author is ommited since the arugments are optional. In Short's case, the original url is always required during creation, so having one input where it's both optional for both creation and mutation would be misleading when using the schema. We could have one single input type where all fields are optional and enforce requirements in the implemenation, but it might lead to confusion down the road.

So far these are the two solutions with their tradeoffs: Solution Pros Cons
One Input Reusability and simplicity Schema might cause confusion as to what arguments are required for each use case
Two Inputs Explicit attribute requirements Future addition to one input type would require modification of two input types the update input and creation input.

After going over the tradeoffs, I think that one input might be the better option at this time. The chances of causing confusion will be very low if the error handling explicitly describe failure reasons as it is now. However, I am open to either option and the changes required to update the current work on this feature will be very little regardless of the option taken. What are your thoughts?

magicoder10 commented 4 years ago

@oatovar It's a hard a decision. We can go with 1 input for now and look back in the future.