-
Notifications
You must be signed in to change notification settings - Fork 15
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
Handle public-id and system-id xmlfile conditions #87
Conversation
XML buildin rule supported only "matches" option, enabling system-id and public-id. Related to konveyor/analyzer-lsp#221 Signed-off-by: Marek Aufart <maufart@redhat.com>
pkg/conversion/convert.go
Outdated
if xf.Publicid != "" { // <xmlfile public-id=".*JBoss.+DTD Java EE.+5.*"/> | ||
matches := strings.Replace(xf.Matches, "windup:", "", -1) | ||
xmlCond = map[string]interface{}{ | ||
"xpath": "//*[@public-id='regexp:"+substituteWhere(where, matches)+"']", |
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.
xpath module in analyzer doesn't look to support regexps. Considering update https://github.com/konveyor/analyzer-lsp/blob/main/provider/internal/builtin/service_client.go#L181 with someling like:
- if the xpath starts with
//*[@public-id='regexp:
, take the reg.expression and update xpath to//*[@public-id]
- search for the updated xpath (all nodes with public-id attribute)
- iterate found nodes in loop and check if regexp matches (by golang)
Other option could be extend xmlCond struct to store regexp there, but I'm open to all possible suggestions!
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.
I lean towards storing the regex in the xmlCond explicitly, but if the behavior of pulling it out automatically is consistently unsurprising I'd be cool with that too.
@shawn-hurley thoughts?
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.
After thinking about this,
- having some regexp interspersed, for a particular submatch makes me feel like too much magic.
- Looks like matches and system-id's will work, without the need for regex
I would vote for a whole new XML cond, xml-publicid or something, that has a regexp as a field. This way it is clear the only thing that will do a regex submatch is this particular condition?
What do you all think? Am I missing something?
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.
@shawn-hurley that sounds good to me too
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.
Understood, going to make a xml-publicid condition.
Overall success rate: 80.12% (1894/2364) |
Adding XML-like public-id condition supporting regular expression evaluation on the public-id attribute. Related to konveyor/windup-shim#87 Related to konveyor#221 Signed-off-by: Marek Aufart <maufart@redhat.com>
Signed-off-by: Marek Aufart <maufart@redhat.com>
Signed-off-by: Marek Aufart <maufart@redhat.com>
Signed-off-by: Marek Aufart <maufart@redhat.com>
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.
/lgtm assume we need to re-run after: konveyor/analyzer-lsp#306 is added?
Adding XML-like public-id condition supporting regular expression evaluation on the public-id attribute. Related to konveyor/windup-shim#87 Fixes: #221 Analyzer PR: 306 Windup-shim PR: 87 --------- Signed-off-by: Marek Aufart <maufart@redhat.com> Signed-off-by: Marek Aufart <aufi.cz@gmail.com>
Re-executing integration tests. |
konveyor/analyzer-lsp#306 was merged, tests are green, merging. |
XML buildin rule supported only "matches" option, enabling system-id and public-id.
Related to konveyor/analyzer-lsp#221