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 ability to set JSON encoded Swagger extension values #788

Merged
merged 1 commit into from Sep 22, 2016

Conversation

Projects
None yet
2 participants
@raphael
Copy link
Member

raphael commented Sep 22, 2016

No description provided.

@raphael

This comment has been minimized.

Copy link
Member

raphael commented Sep 22, 2016

@tchssk fyi, you can now encode values in JSON in the Metadata field for Swagger extensions.

@raphael raphael merged commit 3bb8908 into master Sep 22, 2016

3 checks passed

continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@raphael raphael deleted the json_swagger_extension_value branch Sep 22, 2016

@tchssk

This comment has been minimized.

Copy link
Member

tchssk commented Sep 23, 2016

Great! Thank you for telling me. @raphael

@tchssk

This comment has been minimized.

Copy link
Member

tchssk commented Sep 23, 2016

I read the document of Swagger Extensions again.
It includes follwing line.

Extension properties are always prefixed by "x-" and can have any valid JSON format value.

If it means

Extension properties are always prefixed by "x-" and can have only any valid JSON format value.

maybe we should have picked up only valid values as JSON and simply stored them as string like below.

func extensionsFromDefinition(mdata dslengine.MetadataDefinition) map[string]string {
    extensions := make(map[string]string)
    for key, value := range mdata {
        chunks := strings.Split(key, ":")
        if len(chunks) != 3 {
            continue
        }
        if chunks[0] != "swagger" || chunks[1] != "extension" {
            continue
        }
        if strings.HasPrefix(chunks[2], "x-") != true {
            continue
        }
        // Validate value.
        var unmarshaled interface{}
        if err := json.Unmarshal(value[0], &unmarshaled); err != nil {
            // Skip if invalid.
            continue
        }
        // Set if valid.
        extensions[chunks[2]] = value[0]
    }
    if len(extensions) == 0 {
        return nil
    }
    return extensions
}

How do you think? @raphael

@raphael

This comment has been minimized.

Copy link
Member

raphael commented Sep 23, 2016

Hmm yes that's a good point. It's going to be less practical for strings though as you'd have to double escape them. I could see people getting bitten by this. Maybe we could keep doing what the algorithm does today and if json unmarshal fails then fall back to using the value directly. It's not great from a performance point of view but this is code generation so it's fine :)

@tchssk

This comment has been minimized.

Copy link
Member

tchssk commented Sep 23, 2016

I almost understood.
Thank you for your reply!

@raphael

This comment has been minimized.

Copy link
Member

raphael commented Sep 23, 2016

Yeah my response wasn't super clear now that I re-read it :)

What I mean to say is that like you suggest we should remove the json: prefix and always do json.Unmarshal on the value. However if that fails I think we shouldn't fail entirely and instead just use the value as is. So I can still write:

Metadata("swagger:extension:x-name", "string value")

I don't have to do:

Metadata("swagger:extension:x-name", `"string value"`)

But that would work too.

@tchssk

This comment has been minimized.

Copy link
Member

tchssk commented Sep 24, 2016

Oh I understand.

And I was planned to undo each Extensions to map[string]string but they may have to keep being currently type. A value may have to be stored as interface{] when it valid and as string when it invalid.

@raphael

This comment has been minimized.

Copy link
Member

raphael commented Sep 24, 2016

cool! yeah I think the Extensions fields have to be map[string]interface{}

@tchssk

This comment has been minimized.

Copy link
Member

tchssk commented Sep 24, 2016

OK. I'll send a new pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment