short icon indicating copy to clipboard operation
short copied to clipboard

Add GraphQL API to update the mutated short links

Open andyyaldoo opened this issue 5 years ago • 13 comments

From #237

andyyaldoo avatar Oct 13 '19 03:10 andyyaldoo

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

oatovar avatar Mar 23 '20 05:03 oatovar

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 avatar Mar 29 '20 21:03 oatovar

GraphQL Schema

type AuthMutation {
    createURL(url: URLInput!, isPublic: Boolean!): URL
    updateURL(alias: String!, url: URLInput!): URL
    deleteURL(alias: String!)
}

oatovar avatar Mar 30 '20 00:03 oatovar

@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.

magicoder10 avatar Mar 30 '20 06:03 magicoder10

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

oatovar avatar Apr 01 '20 03:04 oatovar

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

oatovar avatar Apr 14 '20 00:04 oatovar

@oatovar I think so.

magicoder10 avatar Apr 14 '20 00:04 magicoder10

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).

oatovar avatar May 17 '20 05:05 oatovar

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

magicoder10 avatar May 17 '20 05:05 magicoder10

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.

oatovar avatar May 17 '20 07:05 oatovar

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

magicoder10 avatar May 17 '20 21:05 magicoder10

@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?

oatovar avatar May 18 '20 02:05 oatovar

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

magicoder10 avatar May 21 '20 00:05 magicoder10