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

[New Feature] Problem Details for HTTP APIs #1335

Closed
kataras opened this issue Aug 11, 2019 · 12 comments

Comments

@kataras
Copy link
Owner

commented Aug 11, 2019

Based on:

  1. RFC https://tools.ietf.org/html/rfc7807
  2. Best implementation of this RFC is done by a Java library, https://github.com/zalando/problem. I will (try to) port all features. Go has no decent library for this, neither any web framework based on Go. Iris will be the first on this too.
  • Structure: Problem { type, title, status, detail, cause: Problem{ ... }, $any_custom }
  • Content Type: application/problem+json

For the upcoming Iris v11.2.5

kataras added a commit that referenced this issue Aug 12, 2019

@kataras

This comment has been minimized.

Copy link
Owner Author

commented Aug 12, 2019

Done with: #1336

@Dexus

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2019

Hi @kataras
should this not also return the http header for the status?

@kataras

This comment has been minimized.

Copy link
Owner Author

commented Aug 14, 2019

@Dexus I did that on the initial implementation but then I re-written and forgot to send the status code, you are right, it's done now.

kataras added a commit that referenced this issue Aug 14, 2019

@kataras kataras closed this Aug 14, 2019

@Dexus

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2019

@kataras thanks.

Problem type definitions MAY specify the use of the Retry-After
response header ([RFC7231], Section 7.1.3) in appropriate
circumstances.

Can you also add this, too? Like:
NewProblem().Retry(timeSeconds)

@kataras

This comment has been minimized.

Copy link
Owner Author

commented Aug 14, 2019

I would love, although the java implementation (the most complete we know) missing that as I could see, but don't forget you can always add a PR, your notes are quite helpful!

The Problem type is map[string]interface{}, we can't save properties inside there, the Retry-After is a response header and should be defined somewhere else, I am thinking two ways:

  1. provide options on the ctx.Problem(problem, ..here)
  2. make a temp key on problem map, set the header based on that key-value and remove it on execution so it doesn't send to client(the status should be sent to client but retry-after no)
@Dexus

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2019

Option 1 would be another braking change. Currently only context.JSON{} is accepted as option.

Option 2 would be my favorite, because we can use a key that violates the RFC. Otherwise only alphanumeric and "_" are allowed.

If such additional members are defined, their names SHOULD start with
a letter (ALPHA, as per [RFC5234], Appendix B.1) and SHOULD consist
of characters from ALPHA, DIGIT ([RFC5234], Appendix B.1), and "_"
(so that it can be serialized in formats other than JSON), and they
SHOULD be three characters or longer.

I could imagine myself using a key like "::OPT::" internally. Even if it wouldn't be ideal for performance, because of the special characters.

... but don't forget you can always add a PR, your notes are quite helpful!

Thanks, after my holiday I will gladly submit a PR for possible improvements/ideas.

@kataras

This comment has been minimized.

Copy link
Owner Author

commented Aug 14, 2019

The first implementation on setting them on the map itself, looks like this:

const (
	problemTempKeyPrefix = "@temp_"
	retryAfterHeaderKey  = "Retry-After"
)

// No need, let's remove them on receive, once.
//
// func (p Problem) removeTempProperties() {
// 	for k, v := range p {
// 		if strings.HasPrefix(k, problemTempKeyPrefix) {
// 			delete(p, k)
// 			continue
// 		}
//
// 		if k == "cause" {
// 			if causeP, ok := v.(Problem); ok {
// 				causeP.removeTempProperties()
// 			}
// 		}
// 	}
// }

// TempKey sets a temporary key-value pair, which is being removed
// on the its first get.
func (p Problem) TempKey(key string, value interface{}) Problem {
	return p.Key(problemTempKeyPrefix+key, value)
}

func (p Problem) getTempKey(key string) interface{} {
	key = problemTempKeyPrefix + key
	v, ok := p[key]
	if ok {
		delete(p, key)
		return v
	}

	return nil
}

func parseRetryAfter(value interface{}, timeLayout string) string {
	// https://tools.ietf.org/html/rfc7231#section-7.1.3
	// Retry-After = HTTP-date / delay-seconds
	switch v := value.(type) {
	case time.Time:
		return v.Format(timeLayout)
	case int64:
		return strconv.FormatInt(v, 10)
	case string:
		dur, err := time.ParseDuration(v)
		if err != nil {
			t, err := time.Parse(timeLayout, v)
			if err != nil {
				return ""
			}

			return parseRetryAfter(t, timeLayout)
		}

		return parseRetryAfter(parseDurationToSeconds(dur), timeLayout)
	}

	return ""
}

func (p Problem) getRetryAfter(timeLayout string) string {
	value := p.getTempKey(retryAfterHeaderKey)
	if value == nil {
		return ""
	}

	return parseRetryAfter(value, timeLayout)
}

func parseDurationToSeconds(dur time.Duration) int64 {
	return int64(math.Round(dur.Seconds()))
}

// RetryAfter sets a temp key to be set on Retry-After response header.
// https://tools.ietf.org/html/rfc7231#section-7.1.3
// The value can be one of those:
// time.Time
// time.Duration for seconds
// int64, int, float64 for seconds
// string for duration string or for datetime string.
func (p Problem) RetryAfter(dateOrDurationOrIntSecondsOrString interface{}) Problem {
	switch v := dateOrDurationOrIntSecondsOrString.(type) {
	case time.Time:
		return p.TempKey(retryAfterHeaderKey, v)
	case time.Duration:
		return p.TempKey(retryAfterHeaderKey, parseDurationToSeconds(v))
	case int64:
		return p.TempKey(retryAfterHeaderKey, v)
	case int:
		return p.TempKey(retryAfterHeaderKey, int64(v))
	case float64:
		return p.TempKey(retryAfterHeaderKey, int64(math.Round(v)))
	case string:
		return p.TempKey(retryAfterHeaderKey, v)
	default:
		// panic?
	}

	return p
}

And context gets the retry header and set it.

However, with this implementation we have an issue of a Problem re-use, the caller should re-set the RetryAfter this is probably OK for that header as it needs to be relative to the execution time and not the type creation, let's hear your thoughts first before I push it later on.

@kataras kataras reopened this Aug 14, 2019

@Dexus

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2019

Looks good so far.

In the function func (p Problem) RetryAfter I would set 5 minutes as default, this is usually a sufficient value. At least in the APIs we use and offer here.

That the func (p Problem) RetryAfter has to be re-set with every delivery is okay. Alternatively, there would be an option to add whether this should be permanent - which would only have a real added value if seconds were specified - but on the other hand would not be solved with a Temporary Key.

I'm having a hard time to look deeper and understand the code with my mobile phone in hand at the moment.

https://github.com/kataras/iris/blob/master/context/problem.go#L80

Think this should also be a loop over the p["cause"] of every child. Because it's multi-level.

@kataras

This comment has been minimized.

Copy link
Owner Author

commented Aug 15, 2019

but on the other hand would not be solved with a Temporary Key.

Then we will have to clone the Problem, remove this key, and push it as JSON so the caller's Problem don't change, this will have a performance cost (cloning the map).

Think this should also be a loop over the p["cause"] of every child. Because it's multi-level.

What do you mean? to check for a valid retry-after to all children or take the maximum or add all of them?

That's why I think to make it as Options, currently it accepts JSON options but we can wrap to a new ProblemOptions for things like that, meanwhile the end-dev can set the ctx.Header("Retry-After", value).

@kataras

This comment has been minimized.

Copy link
Owner Author

commented Aug 15, 2019

@Dexus

I made it with options, even if we have a 2-days breaking change, the fix is easy just pass the prev JSON on the ProblemOptions.JSON field. It's far better and can be re-used by end-dev and on the future for more options. No default values there except the json indentation to " ".

	ctx.Problem(newProductProblem("product name", "problem details"), iris.ProblemOptions{
		// Optional JSON renderer settings.
		JSON: iris.JSON{
			Indent: "  ",
		},
		// Can accept:
		// time.Time for HTTP-Date,
		// time.Duration, int64, float64, int for seconds
		// or string for date or duration.
		// Examples:
		// time.Now().Add(5 * time.Minute),
		// 300 * time.Second,
		// "5m",
		//
		RetryAfter: 300,
		// A function that, if specified, can dynamically set
		// retry-after based on the request. Useful for ProblemOptions reusability.
		// Overrides the RetryAfter field.
		//
		// RetryAfterFunc: func(iris.Context) interface{} { [...]}
	})
}

The Problem type itself still has TempKey to set a temp key and GetTempKey to retrieve and delete it for anyone who may need it.

kataras added a commit that referenced this issue Aug 15, 2019

kataras added a commit that referenced this issue Aug 15, 2019

@Dexus

This comment has been minimized.

Copy link
Contributor

commented Aug 15, 2019

@kataras

https://github.com/kataras/iris/blob/master/context/problem.go#L80
Think this should also be a loop over the p["cause"] of every child. Because it's multi-level.

What do you mean? to check for a valid retry-after to all children or take the maximum or add all of them?

That a problem can be a cause problem cause problem cause cause problem, i.e. multi level problems. Which not only have 2 levels.
It's okay, it's calling recusive. I was wrong yesterday on my reading.

but on the other hand would not be solved with a Temporary Key.

Then we will have to clone the Problem, remove this key, and push it as JSON so the caller's Problem don't change, this will have a performance cost (cloning the map).

Otherwise, I agree with you, is fair enough, for 2 days, to use the new options. So it has less impact on performance.

@kataras

This comment has been minimized.

Copy link
Owner Author

commented Aug 16, 2019

Yes, I think it's better that way @Dexus. Happy holidays man, you are quite lucky :)

@kataras kataras closed this Aug 16, 2019

kataras added a commit that referenced this issue Aug 16, 2019

New XMLMap func which makes a map value type an xml compatible conten…
…t to render and give option to render a Problem as XML - rel to #1335
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.