Skip to content
Open
Show file tree
Hide file tree
Changes from 3 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,143 @@
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;
}
// 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(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 not use get(0) as this will only check the first element in the list of configured rules.

Copy link
Owner Author

Choose a reason for hiding this comment

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

There's only one entry and its in String form.

We ended up with .get(0) for cases where there's only one String listedItemToCheck, but other rules have a list listedItemToCheck so instead of having a RULE_CONFIG attribute for String ItemToCheck and List listedItemToCheck we can just keep the list attribute, and when a String is encountered, it returns a list with one entry

}

/**
* 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 "typesToCheck":
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="WARNING"
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": {
"typesToCheck": "implementation",
Copy link

Choose a reason for hiding this comment

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

This is not really a "type". It's a sub-package name. So, we should consider changing the name here.

Copy link
Owner Author

Choose a reason for hiding this comment

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

done - using subPackageName now

"antiPatternMessage": "Detected usage of an implementation type. Implementation types are not intended for public use. Use the publicly available Azure classes instead."
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,196 @@
package com.microsoft.azure.toolkit.intellij.azure.sdk.buildtool;

import com.intellij.codeInspection.ProblemsHolder;
import com.intellij.psi.JavaElementVisitor;
import com.intellij.psi.PsiClass;
import com.intellij.psi.PsiClassType;
import com.intellij.psi.PsiIdentifier;
import com.intellij.psi.PsiVariable;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.mockito.Mock;

import com.microsoft.azure.toolkit.intellij.azure.sdk.buildtool.ImplementationTypeCheck.ImplementationTypeVisitor;
import org.mockito.Mockito;

import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;

/**
* This class tests the ImplementationTypeVisitor class.
*/
public class ImplementationTypeCheckTest {

// Declare as instance variables
@Mock
private ProblemsHolder mockHolder;

@Mock
private JavaElementVisitor mockVisitor;

@Mock
private PsiVariable mockVariable;

@BeforeEach
public void setup() {
mockHolder = mock(ProblemsHolder.class);
mockVisitor = createVisitor();
mockVariable = mock(PsiVariable.class);
}

/**
* Test cases for the ImplementationTypeVisitor class.
* This case is for a class that is an implementation type.
* The class is in the implementation package.
* The registerProblem method should be called.
*/
@Test
public void testDirectUseOfImplementationType() {
String classQualifiedName = "com.azure.data.appconfiguration.implementation.models";

// No interfaces implemented
PsiClass[] interfaces = new PsiClass[]{};
String interfaceQualifiedName = null;

// Extends Object (implicitly)
PsiClass superClass = mock(PsiClass.class);
String superClassQualifiedName = "java.lang.Object"; // Extends java.lang.Object

int numOfInvocations = 1;
verifyRegisterProblem(classQualifiedName, interfaces, superClass, superClassQualifiedName, interfaceQualifiedName, numOfInvocations);
}

/**
* Test cases for the ImplementationTypeVisitor class.
* This case is for a class that is not an implementation type.
* The registerProblem method should not be called.
*/
@Test
public void testUseOfNonImplementationAzurePackage() {
String classQualifiedName = "com.azure.data.appconfiguration.models";

// No interfaces implemented
PsiClass[] interfaces = new PsiClass[]{};
String interfaceQualifiedName = null;

// Extends Object (implicitly)
PsiClass superClass = mock(PsiClass.class);
String superClassQualifiedName = "java.lang.Object"; // Extends java.lang.Object

int numOfInvocations = 0;
verifyRegisterProblem(classQualifiedName, interfaces, superClass, superClassQualifiedName, interfaceQualifiedName, numOfInvocations);
}

/**
* Test cases for the ImplementationTypeVisitor class.
* This case is for a class that implements an implementation type interface.
* The registerProblem method should be called.
*/
@Test
public void testUseOfImplementationTypeInterface() {
String classQualifiedName = "com.azure.data.appconfiguration.models";

// Implements an implementation type interface
PsiClass interfaceClass = mock(PsiClass.class);
String interfaceQualifiedName = "com.azure.data.appconfiguration.implementation.models";
PsiClass[] interfaces = new PsiClass[]{interfaceClass};

// Extends Object (implicitly)
PsiClass superClass = mock(PsiClass.class);
String superClassQualifiedName = "java.lang.Object"; // Extends java.lang.Object

int numOfInvocations = 1;
verifyRegisterProblem(classQualifiedName, interfaces, superClass, superClassQualifiedName, interfaceQualifiedName, numOfInvocations);
}

/**
* Test cases for the ImplementationTypeVisitor class.
* This case is for a class that extends an implementation type abstract class.
* The registerProblem method should be called.
*/
@Test
public void testUseOfImplementationTypeAbstractClass() {
String classQualifiedName = "com.azure.data.appconfiguration.models";

// Implements an implementation type interface
String interfaceQualifiedName = null;
PsiClass[] interfaces = new PsiClass[]{};

// Extends Object (implicitly)
PsiClass superClass = mock(PsiClass.class);
String superClassQualifiedName = "com.azure.data.appconfiguration.implementation.models"; // Extends java.lang.Object

int numOfInvocations = 1;
verifyRegisterProblem(classQualifiedName, interfaces, superClass, superClassQualifiedName, interfaceQualifiedName, numOfInvocations);
}

/**
* Test cases for the ImplementationTypeVisitor class.
* This case is for a non-Azure class.
* The registerProblem method should not be called.
*/
@Test
public void testUseOfNonAzurePackage() {
String classQualifiedName = "com.nonazure.data.appconfiguration.implementation.models";

// No interfaces implemented
PsiClass[] interfaces = new PsiClass[]{};
String interfaceQualifiedName = null;

// Extends Object (implicitly)
PsiClass superClass = mock(PsiClass.class);
String superClassQualifiedName = "java.lang.Object"; // Extends java.lang.Object

int numOfInvocations = 0;
verifyRegisterProblem(classQualifiedName, interfaces, superClass, superClassQualifiedName, interfaceQualifiedName, numOfInvocations);
}

/**
* Helper method to create visitor.
*
* @return ImplementationTypeVisitor
*/
JavaElementVisitor createVisitor() {
return new ImplementationTypeVisitor(mockHolder);
}

/**
* Helper method to verify registerProblem method.
*
* @param classQualifiedName Qualified name of the class
* @param interfaces Array of interfaces implemented by the class
* @param superClass Super class of the class - This is the class that the class extends
* @param superClassQualifiedName Qualified name of the super class
* @param interfaceQualifiedName Qualified name of the interface - This is the interface that the class implements
* @param numOfInvocations Number of times registerProblem method is called
*/
private void verifyRegisterProblem(String classQualifiedName, PsiClass[] interfaces, PsiClass superClass, String superClassQualifiedName, String interfaceQualifiedName, int numOfInvocations) {

PsiClassType type = mock(PsiClassType.class);
PsiClass psiClass = mock(PsiClass.class);
PsiIdentifier mockIdentifier = mock(PsiIdentifier.class);

when(mockVariable.getType()).thenReturn(type);

// isImplementationType method
when(type.resolve()).thenReturn(psiClass);
when(psiClass.getQualifiedName()).thenReturn(classQualifiedName);

// ExtendsOrImplementsImplementationType method
when(psiClass.getInterfaces()).thenReturn(interfaces);
when(psiClass.getSuperClass()).thenReturn(superClass);

when(superClass.getQualifiedName()).thenReturn(superClassQualifiedName);

if (interfaces.length > 0) {
when(interfaces[0].getQualifiedName()).thenReturn(interfaceQualifiedName);
}

when(mockVariable.getNameIdentifier()).thenReturn(mockIdentifier);
mockVisitor.visitVariable(mockVariable);
verify(mockHolder, times(numOfInvocations)).registerProblem(eq(mockIdentifier), Mockito.contains("Detected usage of an implementation type. Implementation types are not intended for public use. Use the publicly available Azure classes instead."));
}
}