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

fix yaml archives for conformance #1206

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
2 changes: 0 additions & 2 deletions cmd/registry/cmd/apply/apply_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -229,8 +229,6 @@ func TestArtifactStorage(t *testing.T) {
}

want := &apihub.Lifecycle{
Id: "lifecycle", // deprecated field
Kind: "Lifecycle", // deprecated field
DisplayName: "Lifecycle",
Description: "A series of stages that an API typically moves through in its lifetime",
Stages: []*apihub.Lifecycle_Stage{
Expand Down
7 changes: 6 additions & 1 deletion cmd/registry/cmd/compute/conformance/conformance.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ import (
"github.com/spf13/cobra"
)

var styleguideFilter = fmt.Sprintf("mime_type.contains('%s')", mime.MimeTypeForKind("StyleGuide"))
var styleguideFilter = fmt.Sprintf("mime_type.contains('%s') || mime_type.contains('%s')", mime.MimeTypeForKind("StyleGuide"), mime.YamlMimeTypeForKind("StyleGuide"))

func Command() *cobra.Command {
var filter string
Expand Down Expand Up @@ -82,6 +82,11 @@ func Command() *cobra.Command {
log.FromContext(ctx).WithError(err).Debugf("Unmarshal() to StyleGuide failed on artifact: %s", artifact.GetName())
return nil
}
name, err := names.ParseArtifact(artifact.GetName())
if err != nil {
return err
}
guide.Id = name.ArtifactID()
guides = append(guides, guide)
return nil
}); err != nil {
Expand Down
82 changes: 17 additions & 65 deletions cmd/registry/patch/artifact.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,6 @@ package patch

import (
"context"
"encoding/json"
"errors"
"fmt"
"strings"

Expand Down Expand Up @@ -115,54 +113,29 @@ func storeArchivesAsYaml(ctx context.Context) bool {
}

func applyArtifactPatch(ctx context.Context, client connection.RegistryClient, content *encoding.Artifact, parent string, filename string) error {
// Restyle the YAML representation so that yaml.Marshal will marshal it as JSON.
encoding.StyleForJSON(&content.Data)
// Marshal the YAML representation into the JSON serialization.
j, err := yaml.Marshal(content.Data)
if err != nil {
return err
}
// Populate Id and Kind fields in the contents of the artifact
jWithIdAndKind, err := populateIdAndKind(j, content.Kind, content.Metadata.Name)
if err != nil {
return err
}
var mimeType string
var bytes []byte
// Unmarshal the JSON serialization into the message struct.
var m proto.Message
m, err = mime.MessageForKind(content.Kind)
if err == nil {
err = protojson.Unmarshal(jWithIdAndKind, m)
msg, err := mime.MessageForKind(content.Kind)
if storeArchivesAsYaml(ctx) || err != nil {
mimeType = mime.YamlMimeTypeForKind(content.Kind)
encoding.StyleForYAML(&content.Data)
bytes, err = yaml.Marshal(content.Data)
if err != nil {
if strings.Contains(err.Error(), "unknown field") {
// Try unmarshaling the original YAML (without the additional Id and Kind fields).
err = protojson.Unmarshal(j, m)
if err != nil {
return err
}
}
}
if storeArchivesAsYaml(ctx) {
mimeType = mime.YamlMimeTypeForKind(content.Kind)
encoding.StyleForYAML(&content.Data)
bytes, err = yaml.Marshal(content.Data)
if err != nil {
return err
}
} else {
mimeType = mime.MimeTypeForKind(content.Kind)
// Marshal the message struct to bytes.
bytes, err = proto.Marshal(m)
if err != nil {
return err
}
return err
}
} else {
// If there was no struct defined for the type, marshal it struct as YAML
// Convert YAML to JSON for protojson
encoding.StyleForJSON(&content.Data)
j, err := yaml.Marshal(content.Data)
if err != nil {
return err
}
err = protojson.Unmarshal(j, msg)
if err != nil {
return err
}
mimeType = mime.MimeTypeForKind(content.Kind)
encoding.StyleForYAML(&content.Data)
bytes, err = yaml.Marshal(content.Data)
bytes, err = proto.Marshal(msg)
if err != nil {
return err
}
Expand Down Expand Up @@ -196,27 +169,6 @@ func applyArtifactPatch(ctx context.Context, client connection.RegistryClient, c
return nil
}

// populateIdAndKind inserts the "id" and "kind" fields in the supplied json bytes.
func populateIdAndKind(bytes []byte, kind, id string) ([]byte, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this will break API hub, which currently reads proto-serialized artifacts and expects them to contain "kind" and "id" fields. Since these fields are redundant in Registry YAML, I don't think we should be including them in YAML-serialized artifacts and I expect that API hub will adapt to that when it starts reading those YAML-serialized artifacts.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's unfortunate. So I need to add this back for protos?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, this PR isn't critical until we go all in on yaml...perhaps we should hold it until we correct API Hub.

var jsonData map[string]interface{}
err := json.Unmarshal(bytes, &jsonData)
if err != nil {
return nil, err
}
if jsonData == nil {
return nil, errors.New("missing data")
}
jsonData["id"] = id
jsonData["kind"] = kind

rBytes, err := json.Marshal(jsonData)
if err != nil {
return nil, err
}

return rBytes, nil
}

func UnmarshalContents(contents []byte, mimeType string, message proto.Message) error {
if !mime.IsYamlKind(mimeType) {
return proto.Unmarshal(contents, message)
Expand Down
3 changes: 0 additions & 3 deletions cmd/registry/patch/patch_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1185,9 +1185,6 @@ func TestInvalidArtifactPatches(t *testing.T) {
{
artifactID: "references-no-data",
},
{
artifactID: "struct-invalid-structure",
},
{
artifactID: "struct-no-metadata",
},
Expand Down

This file was deleted.