Skip to content
Open
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -259,4 +259,13 @@ integration, telemetry connectivity, and Azure Toolkit integration.
that require it. Otherwise, it is not necessary to authenticate non-Azure Open-AI clients.
Please refer to
the [KeyCredential Class documentation](https://learn.microsoft.com/java/api/com.azure.core.credential.keycredential?view=azure-java-stable)
for more information.
for more information.

14. ## Using Implementation Types instead of Publicly Available Azure Classes

**Anti-pattern**: Using implementation types instead of publicly available Azure classes.
- **Issue**: Implementation types are internal classes that are not intended for public use. They may change or be
removed
in future versions, leading to compatibility issues.
- **Severity: WARNING**
- **Recommendation**: Use the publicly available Azure classes instead.
Original file line number Diff line number Diff line change
@@ -0,0 +1,150 @@
package com.microsoft.azure.toolkit.intellij.azure.sdk.buildtool;

import com.intellij.codeInspection.LocalInspectionTool;
import com.intellij.codeInspection.ProblemsHolder;
import com.intellij.psi.JavaElementVisitor;
import com.intellij.psi.PsiClass;
import com.intellij.psi.PsiClassType;
import com.intellij.psi.PsiType;
import com.intellij.psi.PsiVariable;
import org.jetbrains.annotations.NotNull;

/**
* This class is an inspection tool that checks if a variable type is an Azure implementation type.
* If the variable type is an Azure implementation type, or if the variable type extends or implements an Azure implementation type,
* the inspection tool will flag it as a problem.
*/
public class ImplementationTypeCheck extends LocalInspectionTool {

/**
* Build the visitor for the inspection. This visitor will be used to traverse the PSI tree.
*
* @param holder The holder for the problems found
* @param isOnTheFly Whether the inspection is being run on the fly - This is not used in this implementation, but is required by the interface
* @return The visitor for the inspection
*/
@NotNull
@Override
public JavaElementVisitor buildVisitor(@NotNull ProblemsHolder holder, boolean isOnTheFly) {
return new ImplementationTypeVisitor(holder);
}

/**
* This class extends the JavaElementVisitor to visit the elements in the code.
* It checks if the variable type is an Azure implementation type and flags it as a problem if it is.
*/
static class ImplementationTypeVisitor extends JavaElementVisitor {

private final ProblemsHolder holder;

/**
* Constructor for the visitor
*
* @param holder - the ProblemsHolder object to register the problem
*/
ImplementationTypeVisitor(ProblemsHolder holder) {
this.holder = holder;
}

// Define constants for string literals
private static final RuleConfig RULE_CONFIG;
private static final boolean SKIP_WHOLE_RULE;
private static final String RULE_NAME = "ImplementationTypeCheck";

static {
RuleConfigLoader centralRuleConfigLoader = RuleConfigLoader.getInstance();

// Get the RuleConfig object for the rule
RULE_CONFIG = centralRuleConfigLoader.getRuleConfig(RULE_NAME);
SKIP_WHOLE_RULE = RULE_CONFIG.skipRuleCheck() || RULE_CONFIG.getAntiPatternMessageMap().isEmpty() || RULE_CONFIG.getListedItemsToCheck().isEmpty();
}

/**
* This method visits the variables in the code.
* It checks if the variable type is an Azure implementation type and flags it as a problem if it is.
*/
@Override
public void visitVariable(@NotNull PsiVariable variable) {
super.visitVariable(variable);

if (SKIP_WHOLE_RULE) {
return;
}

// Get the type of the variable - This is the type of the variable declaration
// eg. List<String> myList = new ArrayList<>(); -> type = List<String>
PsiType type = variable.getType();

// Check if the type directly used is an implementation type
if (isImplementationType(type) && variable.getNameIdentifier() != null) {
holder.registerProblem(variable.getNameIdentifier(), RULE_CONFIG.getAntiPatternMessageMap().get("antiPatternMessage"));

// Check if the type extends or implements an implementation type
} else if (extendsOrImplementsImplementationType(type) && variable.getNameIdentifier() != null) {
holder.registerProblem(variable.getNameIdentifier(), RULE_CONFIG.getAntiPatternMessageMap().get("antiPatternMessage"));
}
}

/**
* This method checks if the type is a class from the Azure package and if it is an implementation type.
* It returns true if the type is an implementation type and false otherwise.
*/
private boolean isImplementationType(PsiType type) {

if (!(type instanceof PsiClassType)) {
return false;
}
PsiClass psiClass = ((PsiClassType) type).resolve();

if (psiClass == null) {
return false;
}

for (String listedItem : RULE_CONFIG.getListedItemsToCheck()) {
if (psiClass.getQualifiedName() != null && psiClass.getQualifiedName().contains(listedItem)) {

// Check if the class is in the Azure package and if it is an implementation type
return psiClass.getQualifiedName().startsWith(RuleConfig.AZURE_PACKAGE_NAME) && psiClass.getQualifiedName().contains(listedItem);
}
}
return false;
}

/**
* This method checks if the type is a class that extends or implements an implementation type.
* It returns true if the type extends or implements an implementation type and false otherwise.
*/
private boolean extendsOrImplementsImplementationType(PsiType type) {
if (type instanceof PsiClassType) {
PsiClass psiClass = ((PsiClassType) type).resolve();

if (psiClass != null) {

PsiClass[] interfaces = psiClass.getInterfaces();
PsiClass superClass = psiClass.getSuperClass();

// Case 1: Class has no implemented interfaces and does not extend any class
if (interfaces.length == 0 && superClass != null && superClass.getQualifiedName().equals("java.lang.Object")) {
return false;
}

// Case 2: Class has implemented interfaces or extends a class that is an implementation type
if (interfaces.length > 0 || (superClass != null && !superClass.getQualifiedName().startsWith("java."))) {
Copy link

Choose a reason for hiding this comment

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

Why do we need to check this? We should check if any of the parent classes are using Azure types and not check for super types that are in java.* package.

Copy link
Owner Author

@Njeriiii Njeriiii Aug 1, 2024

Choose a reason for hiding this comment

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

this was eliminating types in the java.* package so to check for everything else.
I've refactored the whole extendsOrImplementsImplementationType method to check for chains of superclasses & interfaces now


for (PsiClass iface : interfaces) {

// If the interface is from the Azure package and is an implementation type return true
String ifaceName = iface.getQualifiedName();
return ifaceName != null && ifaceName.startsWith(RuleConfig.AZURE_PACKAGE_NAME) && ifaceName.contains(RULE_CONFIG.getListedItemsToCheck().get(0));
}

// If the super class is from the Azure package and is an implementation type return true
String superClassName = superClass.getQualifiedName();
Copy link

Choose a reason for hiding this comment

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

There can be a chain of superclasses. We should check for the whole chain.

Copy link
Owner Author

Choose a reason for hiding this comment

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

I've refactored the whole extendsOrImplementsImplementationType method to check for chains of superclasses & interfaces now

return superClassName != null && superClassName.startsWith(RuleConfig.AZURE_PACKAGE_NAME) && superClassName.contains(RULE_CONFIG.getListedItemsToCheck().get(0));
Copy link

Choose a reason for hiding this comment

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

We should call the isImplementationType method instead of duplicating the logic.

Copy link
Owner Author

Choose a reason for hiding this comment

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

I've refactored the whole extendsOrImplementsImplementationType method to recursively check for chains of superclasses & interfaces now

}
}
}
return false;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,9 @@ private RuleConfig getRuleConfig(JsonReader reader) throws IOException {
case "regexPatterns":
listedItemsToCheck = getValuesFromJsonReader(reader);
break;
case "subPackageName":
listedItemsToCheck = getListFromJsonArray(reader);
break;
default:
if (fieldName.endsWith("Check")) {
// Move to the next token to process the nested object
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -221,5 +221,19 @@
com.microsoft.azure.toolkit.intellij.azure.sdk.buildtool.EndpointOnNonAzureOpenAIAuthCheck
</class>
</localInspection>

<localInspection
language="JAVA"
shortName="ImplementationTypeCheck"
displayName="Implementation Type Check"
groupName="Azure SDK"
enabledByDefault="true"
level="ERROR"
implementationClass="com.microsoft.azure.toolkit.intellij.azure.sdk.buildtool.ImplementationTypeCheck">

<class>
com.microsoft.azure.toolkit.intellij.azure.sdk.buildtool.ImplementationTypeCheck
</class>
</localInspection>
</extensions>
</idea-plugin>
Original file line number Diff line number Diff line change
Expand Up @@ -117,5 +117,9 @@
],
"servicesToCheck": "KeyCredential",
"antiPatternMessage": "Endpoint should not be used with KeyCredential for non-Azure OpenAI clients"
},
"ImplementationTypeCheck": {
"subPackageName": "implementation",
"antiPatternMessage": "Detected usage of an implementation type. Implementation types are not intended for public use. Use the publicly available Azure classes instead."
}
}
Loading