apigee / apigeelint

Static code analysis for Apigee proxy bundles to encourage API developers to use best practices and avoid anti-patterns.
Apache License 2.0
91 stars 71 forks source link

BUG: CC007 incorrectly errors on valid comparisons #410

Closed MonochromeChameleon closed 7 months ago

MonochromeChameleon commented 8 months ago

https://github.com/apigee/apigeelint/pull/403 introduced checking for Condition statement, but incorrectly flags comparisons between two flow variables as invalid. In the PR description it says:

Unknown or invalid operands. Eg fault.name = null is fine, fault.name = invalid_client is invalid. (The RHS should be quoted)

but if invalid_client were a flow variable, this comparison would be valid.

ssvaidyanathan commented 7 months ago

@MonochromeChameleon - I dont think you have a flow variable on both sides of the operation. In your case invalid_client must be a string for the condition to work

MonochromeChameleon commented 7 months ago

@ssvaidyanathan that's just the example from the error comment, but in the more general case, you should definitely be able to compare two flow variables, e.g.


<Step>
    <Name>rf-invalid-format</Name>
    <Condition>detected_format != required_format</Condition>
</Step>
ghost commented 7 months ago

I'm having a similar issue, same code error but when I use multiline conditions or the "not" operator.

Ex:

<Condition>
    proxy.pathsuffix MatchesPath "example" and 
    request.verb = "GET"
</Condition>

and

<Condition>not(proxy.pathsuffix MatchesPath "example")</Condition>
ssvaidyanathan commented 7 months ago

@MonochromeChameleon - Apigee as a product doesnt support comparing two flow variables from what I understand.

ssvaidyanathan commented 7 months ago

@matnsc - I have submitted a PR to allow not.. For now you can use ! instead of not. The multi line condition should be available in 2.46.3. Can you please try and let us know. Its part of #402

Polaratom commented 7 months ago

@ssvaidyanathan APIGEE does support using two flow variables, In my case I check org and env names

I expect org and env names in resource URI, and then use flow variables organization.name and environment.name against the extracted values form resource URI but due to this it's failing

ssvaidyanathan commented 7 months ago

@ssvaidyanathan APIGEE does support using two flow variables, In my case I check org and env names

I expect org and env names in resource URI, and then use flow variables organization.name and environment.name against the extracted values form resource URI but due to this it's failing

@Polaratom - Can you share an example of what you mean please?

DinoChiesa commented 7 months ago

I have run some tests and found these results for what Apigee accepts on the right-hand-side of operators:

| operator              | RHS                 |
| --------------------- | ------------------- |
| Equals                | literal or variable |
| GreaterThanOrEquals   | literal or variable |
| GreaterThan           | literal or variable |
| LesserThanOrEquals    | literal or variable |
| LesserThan            | literal or variable |
| StartsWith            | literal or variable |
| NotEquals             | literal or variable |
| EqualsCaseInsensitive | literal only        |
| JavaRegex             | literal only        |
| MatchesPath           | literal only        |
| Matches               | literal only        |
ghost commented 7 months ago

@matnsc - I have submitted a PR to allow not.. For now you can use ! instead of not. The multi line condition should be available in 2.46.3. Can you please try and let us know. Its part of #402

I did test the lastest PR (#416) in some of our proxies that use multiline, not (and other named operators) and double equals which previously failed CC007 tests, now it's working as expected.

Latest version (2.46.3): before

PR #416: after

DinoChiesa commented 7 months ago

Excellent, thank you for the confirmation. @MonochromeChameleon - Can we close this one out now?

ssvaidyanathan commented 7 months ago

Just released v2.47.0 Please test using this version and let us know if you find any issues.

MonochromeChameleon commented 7 months ago

Excellent, thank you for the confirmation. @MonochromeChameleon - Can we close this one out now?

Yep, all working for me now - thanks!