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.

Sign up for GitHub

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

Jump to bottom

fix: fixes the filtering logic #110

Merged
merged 6 commits into from
Jul 25, 2024
Merged

fix: fixes the filtering logic #110

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
5d2c41e
fixes the filtering logic
andreaangiolillo Jul 24, 2024
4f1abbc
fixes
andreaangiolillo Jul 24, 2024
54a93c5
Update openapi-foas-dev.json
andreaangiolillo Jul 24, 2024
34fb778
Update openapi-foas-dev.yaml
andreaangiolillo Jul 24, 2024
02ea645
Update hidden_envs_test.go
andreaangiolillo Jul 24, 2024
ee76923
Addressed PR comments
andreaangiolillo Jul 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 6 additions & 28 deletions tools/cli/internal/cli/split/split.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
package split

import (
"encoding/json"
"fmt"
"log"
"strings"
Expand Down Expand Up @@ -45,17 +44,13 @@ func (o *Opts) Run() error {
return err
}

oas := specInfo.Spec
versions := openapi.ExtractVersions(oas)
versions, err := openapi.ExtractVersions(specInfo.Spec, o.env)
if err != nil {
return err
}

for _, version := range versions {
// make a copy of the oas to avoid modifying the original document when applying filters
versionedOas, err := duplicateOas(oas)
if err != nil {
return err
}

filteredOAS, err := o.filter(versionedOas, version)
filteredOAS, err := o.filter(specInfo.Spec, version)
if err != nil {
return err
}
Expand All @@ -72,31 +67,14 @@ func (o *Opts) Run() error {
return nil
}

func duplicateOas(doc *openapi3.T) (*openapi3.T, error) {
// Marshal the original document to JSON
jsonData, err := json.Marshal(doc)
if err != nil {
return nil, fmt.Errorf("failed to marshal original OpenAPI specification: %w", err)
}

// Unmarshal the JSON data into a new OpenAPI document
duplicateDoc := &openapi3.T{}
err = json.Unmarshal(jsonData, duplicateDoc)
if err != nil {
return nil, fmt.Errorf("failed to unmarshal duplicated OpenAPI specification: %w", err)
}

return duplicateDoc, nil
}

func (o *Opts) filter(oas *openapi3.T, version string) (result *openapi3.T, err error) {
log.Printf("Filtering OpenAPI document by version %s", version)
apiVersion, err := apiversion.New(apiversion.WithVersion(version))
if err != nil {
return nil, err
}

return oas, filter.ApplyFilters(oas, filter.NewMetadata(apiVersion, o.env))
return filter.ApplyFilters(oas, filter.NewMetadata(apiVersion, o.env))
}

func (o *Opts) saveVersionedOas(oas *openapi3.T, version string) error {
Expand Down
8 changes: 7 additions & 1 deletion tools/cli/internal/cli/versions/versions.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ type Opts struct {
basePath string
outputPath string
format string
env string
}

func (o *Opts) Run() error {
Expand All @@ -41,7 +42,11 @@ func (o *Opts) Run() error {
return err
}

versions := openapi.ExtractVersions(specInfo.Spec)
versions, err := openapi.ExtractVersions(specInfo.Spec, o.env)
if err != nil {
return err
}

if versions == nil {
return fmt.Errorf("no versions found in the OpenAPI specification")
}
Expand Down Expand Up @@ -118,6 +123,7 @@ func Builder() *cobra.Command {
}

cmd.Flags().StringVarP(&opts.basePath, flag.Spec, flag.SpecShort, "", usage.Spec)
cmd.Flags().StringVar(&opts.env, flag.Environment, "dev", usage.Environment)
cmd.Flags().StringVarP(&opts.outputPath, flag.Output, flag.OutputShort, "", usage.Output)
cmd.Flags().StringVarP(&opts.format, flag.Format, flag.FormatShort, "json", usage.Format)
return cmd
Expand Down
51 changes: 46 additions & 5 deletions tools/cli/internal/openapi/filter/filter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,9 @@
package filter

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

"github.com/getkin/kin-openapi/openapi3"
"github.com/mongodb/openapi/tools/cli/internal/apiversion"
Expand Down Expand Up @@ -74,15 +76,54 @@ func initFilters(oas *openapi3.T, metadata *Metadata) error {
return nil
}

func ApplyFilters(doc *openapi3.T, metadata *Metadata) error {
if err := initFilters(doc, metadata); err != nil {
return err
func ApplyFiltersWithInit(doc *openapi3.T, metadata *Metadata, init func(oas *openapi3.T, metadata *Metadata) []Filter) (*openapi3.T, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

[nit] I think naming this WithInit leaks filter.go logic that is not necessary. Would make more sense to call this ApplyFilter and pass the filter type, maybe? and do the init logic here instead of outside

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

do you think the problem is the name and/or the fact that we are passing a function returning an array of filters?

I can call the function newFilters if we think init is the problem

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated. Let me know what you think

// make a copy of the oas to avoid modifying the original document when applying filters
oas, err := duplicateOas(doc)
if err != nil {
return nil, err
}

filtersWithInit := init(oas, metadata)
for _, filter := range filtersWithInit {
if err := filter.Apply(); err != nil {
return nil, err
}
}
return oas, nil
}

func ApplyFilters(doc *openapi3.T, metadata *Metadata) (*openapi3.T, error) {
// make a copy of the oas to avoid modifying the original document when applying filters
oas, err := duplicateOas(doc)
if err != nil {
return nil, err
}

if err := initFilters(oas, metadata); err != nil {
return nil, err
}

for _, filter := range filters {
if err := filter.Apply(); err != nil {
return err
return nil, err
}
}
return nil
return oas, nil
}

func duplicateOas(doc *openapi3.T) (*openapi3.T, error) {
// Marshal the original document to JSON
jsonData, err := json.Marshal(doc)
if err != nil {
return nil, fmt.Errorf("failed to marshal original OpenAPI specification: %w", err)
}

// Unmarshal the JSON data into a new OpenAPI document
duplicateDoc := &openapi3.T{}
err = json.Unmarshal(jsonData, duplicateDoc)
if err != nil {
return nil, fmt.Errorf("failed to unmarshal duplicated OpenAPI specification: %w", err)
}

return duplicateDoc, nil
}
40 changes: 26 additions & 14 deletions tools/cli/internal/openapi/filter/hidden_envs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,13 @@ type HiddenEnvsFilter struct {
metadata *Metadata
}

func InitHiddenEnvsFilter(oas *openapi3.T, metadata *Metadata) *HiddenEnvsFilter {
andreaangiolillo marked this conversation as resolved.
Show resolved Hide resolved
return &HiddenEnvsFilter{
oas: oas,
metadata: metadata,
}
}

func (f *HiddenEnvsFilter) Apply() error {
// delete hidden paths first before processing
for pathName, pathItem := range f.oas.Paths.Map() {
Expand Down Expand Up @@ -91,8 +98,15 @@ func (f *HiddenEnvsFilter) removeRequestBodyIfHiddenForEnv(operation *openapi3.O
return
}

for _, contentType := range operation.RequestBody.Value.Content {
f.removeContentIfHiddenForEnv(contentType)
for k, contentType := range operation.RequestBody.Value.Content {
if isContentTypeHiddenForEnv := isContentTypeHiddenForEnv(contentType, f.metadata.targetEnv); isContentTypeHiddenForEnv {
log.Printf("Removing contentType: %q because is hidden for target env: %q", contentType.Schema.Ref, f.metadata.targetEnv)
// Remove ContentType if it is hidden for the target environment
delete(operation.RequestBody.Value.Content, k)
} else if contentType.Extensions != nil {
// Remove the Hidden extension from the final OAS
delete(contentType.Extensions, hiddenEnvsExtension)
}
}
}

Expand All @@ -109,19 +123,17 @@ func (f *HiddenEnvsFilter) removeResponseIfHiddenForEnv(operation *openapi3.Oper
if response.Value == nil || response.Value.Content == nil {
continue
}
for _, contentType := range response.Value.Content {
f.removeContentIfHiddenForEnv(contentType)
}
}
}

func (f *HiddenEnvsFilter) removeContentIfHiddenForEnv(contentType *openapi3.MediaType) {
if isContentTypeHiddenForEnv := isContentTypeHiddenForEnv(contentType, f.metadata.targetEnv); isContentTypeHiddenForEnv {
log.Printf("Removing contentType: %q because is hidden for target env: %q", contentType.Schema.Ref, f.metadata.targetEnv)
contentType.Schema = nil // Remove ContentType if it is hidden for the target environment
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

contentType.Schema = nil // Remove ContentType if it is hidden for the target environment This piece of code was removing the schema instead of the ContentType

Copy link
Collaborator

Choose a reason for hiding this comment

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

but why are we breaking the patterns you stablished in this filter with the f.removeContentIfHiddenForEnv?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The main reason was that we need to pass the k and response.Value.Content as we need them to delete the content.

} else if contentType.Extensions != nil {
// Remove the Hidden extension from the final OAS
delete(contentType.Extensions, hiddenEnvsExtension)
for k, contentType := range response.Value.Content {
if isContentTypeHiddenForEnv := isContentTypeHiddenForEnv(contentType, f.metadata.targetEnv); isContentTypeHiddenForEnv {
log.Printf("Removing contentType: %q because is hidden for target env: %q", contentType.Schema.Ref, f.metadata.targetEnv)
// Remove ContentType if it is hidden for the target environment
delete(response.Value.Content, k)
} else if contentType.Extensions != nil {
// Remove the Hidden extension from the final OAS
delete(contentType.Extensions, hiddenEnvsExtension)
}
}
}
}

Expand Down
Loading
Loading