-
Notifications
You must be signed in to change notification settings - Fork 45
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 XMLPublicID condition to builtin provider #306
Changes from 6 commits
b835111
6e4cef1
13ff6e3
7ef506c
b1f1500
488c3cb
86d566b
57d16cc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
<?xml version="1.0"?> | ||
<jboss-app> | ||
<loader-repository> | ||
</loader-repository> | ||
<module public-id="JBoss DTD Java EE 5"> | ||
<service>jboss-example-service</service> | ||
</module> | ||
</jboss-app> |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -124,46 +124,19 @@ func (p *builtinServiceClient) Evaluate(ctx context.Context, cap string, conditi | |
if query == nil || err != nil { | ||
return response, fmt.Errorf("could not parse provided xpath query '%s': %v", cond.XML.XPath, err) | ||
} | ||
//TODO(fabianvf): how should we scope the files searched here? | ||
var xmlFiles []string | ||
patterns := []string{"*.xml", "*.xhtml"} | ||
xmlFiles, err = provider.GetFiles(p.config.Location, cond.XML.Filepaths, patterns...) | ||
xmlFiles, err := findXMLFiles(p.config.Location, cond.XMLPublicID.Filepaths) | ||
if err != nil { | ||
return response, fmt.Errorf("unable to find files using pattern `%s`: %v", patterns, err) | ||
return response, fmt.Errorf("Unable to find XML files: %v", err) | ||
} | ||
|
||
for _, file := range xmlFiles { | ||
f, err := os.Open(file) | ||
nodes, err := queryXMLFile(file, query) | ||
if err != nil { | ||
fmt.Printf("unable to open file '%s': %v\n", file, err) | ||
continue | ||
} | ||
// TODO This should start working if/when this merges and releases: https://github.com/golang/go/pull/56848 | ||
var doc *xmlquery.Node | ||
doc, err = xmlquery.ParseWithOptions(f, xmlquery.ParserOptions{Decoder: &xmlquery.DecoderOptions{Strict: false}}) | ||
if err != nil { | ||
if err.Error() == "xml: unsupported version \"1.1\"; only version 1.0 is supported" { | ||
// TODO HACK just pretend 1.1 xml documents are 1.0 for now while we wait for golang to support 1.1 | ||
b, err := os.ReadFile(file) | ||
if err != nil { | ||
fmt.Printf("unable to parse xml file '%s': %v\n", file, err) | ||
continue | ||
} | ||
docString := strings.Replace(string(b), "<?xml version=\"1.1\"", "<?xml version = \"1.0\"", 1) | ||
doc, err = xmlquery.Parse(strings.NewReader(docString)) | ||
if err != nil { | ||
fmt.Printf("unable to parse xml file '%s': %v\n", file, err) | ||
continue | ||
} | ||
} else { | ||
fmt.Printf("unable to parse xml file '%s': %v\n", file, err) | ||
continue | ||
} | ||
} | ||
list := xmlquery.QuerySelectorAll(doc, query) | ||
if len(list) != 0 { | ||
if len(nodes) != 0 { | ||
response.Matched = true | ||
for _, node := range list { | ||
for _, node := range nodes { | ||
ab, err := filepath.Abs(file) | ||
if err != nil { | ||
ab = file | ||
|
@@ -187,6 +160,52 @@ func (p *builtinServiceClient) Evaluate(ctx context.Context, cap string, conditi | |
} | ||
} | ||
|
||
return response, nil | ||
case "xmlPublicID": | ||
regex, err := regexp.Compile(cond.XMLPublicID.Regex) | ||
if err != nil { | ||
return response, fmt.Errorf("Could not parse provided public-id regex '%s': %v", cond.XMLPublicID.Regex, err) | ||
} | ||
query, err := xpath.CompileWithNS("//*[@public-id]", cond.XMLPublicID.Namespaces) | ||
if query == nil || err != nil { | ||
return response, fmt.Errorf("Could not parse public-id xml query '%s': %v", cond.XML.XPath, err) | ||
} | ||
xmlFiles, err := findXMLFiles(p.config.Location, cond.XMLPublicID.Filepaths) | ||
if err != nil { | ||
return response, fmt.Errorf("Unable to find XML files: %v", err) | ||
} | ||
|
||
for _, file := range xmlFiles { | ||
nodes, err := queryXMLFile(file, query) | ||
if err != nil { | ||
continue | ||
} | ||
|
||
for _, node := range nodes { | ||
// public-id attribute regex match check | ||
for _, attr := range node.Attr { | ||
if attr.Name.Local == "public-id" { | ||
if regex.MatchString(attr.Value) { | ||
response.Matched = true | ||
ab, err := filepath.Abs(file) | ||
if err != nil { | ||
ab = file | ||
} | ||
response.Incidents = append(response.Incidents, provider.IncidentContext{ | ||
FileURI: uri.File(ab), | ||
Variables: map[string]interface{}{ | ||
"matchingXML": node.OutputXML(false), | ||
"innerText": node.InnerText(), | ||
"data": node.Data, | ||
}, | ||
}) | ||
} | ||
break | ||
} | ||
} | ||
} | ||
} | ||
|
||
return response, nil | ||
case "json": | ||
query := cond.JSON.XPath | ||
|
@@ -350,6 +369,47 @@ func findFilesMatchingPattern(root, pattern string) ([]string, error) { | |
return matches, err | ||
} | ||
|
||
func findXMLFiles(baseLocation string, filePaths []string) (xmlFiles []string, err error) { | ||
patterns := []string{"*.xml", "*.xhtml"} | ||
// TODO(fabianvf): how should we scope the files searched here? | ||
xmlFiles, err = provider.GetFiles(baseLocation, filePaths, patterns...) | ||
return | ||
} | ||
|
||
func queryXMLFile(filePath string, query *xpath.Expr) (nodes []*xmlquery.Node, err error) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Another one of the small things is that we don't have to change for this PR, but we usually don't use naked returns for a function that has some complexity. In this case, because of the err != nil w/ checking the err on line 390, I think all the shadowing will add complexity and make it slightly harder to reason about. I don't think we have to change this, but I want to point it out. I love to hear thoughts if you disagree just to hear a different perspective; for now, just leave it as is, and we can always address in a follow up :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated returns to use values directly (I didn't realize it is not used in analyzer in that way). I'd plan to address mentioned issues when removing TODOs from golang XML parsing from the code. |
||
f, err := os.Open(filePath) | ||
if err != nil { | ||
fmt.Printf("unable to open file '%s': %v\n", filePath, err) | ||
return | ||
} | ||
defer f.Close() | ||
// TODO This should start working if/when this merges and releases: https://github.com/golang/go/pull/56848 | ||
var doc *xmlquery.Node | ||
doc, err = xmlquery.ParseWithOptions(f, xmlquery.ParserOptions{Decoder: &xmlquery.DecoderOptions{Strict: false}}) | ||
if err != nil { | ||
if err.Error() == "xml: unsupported version \"1.1\"; only version 1.0 is supported" { | ||
// TODO HACK just pretend 1.1 xml documents are 1.0 for now while we wait for golang to support 1.1 | ||
var b []byte | ||
b, err = os.ReadFile(filePath) | ||
if err != nil { | ||
fmt.Printf("unable to parse xml file '%s': %v\n", filePath, err) | ||
return | ||
} | ||
docString := strings.Replace(string(b), "<?xml version=\"1.1\"", "<?xml version = \"1.0\"", 1) | ||
doc, err = xmlquery.Parse(strings.NewReader(docString)) | ||
if err != nil { | ||
fmt.Printf("unable to parse xml file '%s': %v\n", filePath, err) | ||
return | ||
} | ||
} else { | ||
fmt.Printf("unable to parse xml file '%s': %v\n", filePath, err) | ||
return | ||
} | ||
} | ||
nodes = xmlquery.QuerySelectorAll(doc, query) | ||
return | ||
} | ||
|
||
type walkResult struct { | ||
positionParams protocol.TextDocumentPositionParams | ||
match string | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI @aufi, in the future, unless this is used by other functions, I don't know if this extra layer for functions is necessary. It doesn't hurt, but does add a layer to reading what the main function does.
I don't think we need to change or anything, but I'm just giving my perspective and wondering what your thoughts are. I think, for now, it is perfectly fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, your comment makes a good sense to me. The reason for this function was to share the function for XML&XML PublicID conditions (requested in one of previous reviews).