Skip to content
This repository has been archived by the owner on Mar 6, 2023. It is now read-only.

Add an inspection that reports properties being set to their default value #169

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions res/META-INF/plugin.xml
Original file line number Diff line number Diff line change
Expand Up @@ -271,6 +271,10 @@
key="incorrect.variable.type.inspection.display.name"
groupKey="terraform.files.inspection.group.display.name" enabledByDefault="true" level="WARNING"
implementationClass="org.intellij.plugins.hcl.terraform.config.inspection.TFMissingModuleInspection"/>
<localInspection language="HCL" applyToDialects="true" shortName="TFDefaultProperty" bundle="messages.HCLBundle"
key="default.property.inspection.display.name"
groupKey="terraform.files.inspection.group.display.name" enabledByDefault="true" level="WEAK WARNING"
implementationClass="org.intellij.plugins.hcl.terraform.config.inspection.TFDefaultPropertyInspection"/>

<!--region TF Duplicates-->
<localInspection language="HCL" applyToDialects="true" shortName="TFDuplicatedProvider" bundle="messages.HCLBundle"
Expand Down
8 changes: 8 additions & 0 deletions res/inspectionDescriptions/TFDefaultProperty.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
<html>
<body>
Reports properties that are set to their default value
<p>
<!-- tooltip end -->
<p>
</body>
</html>
1 change: 1 addition & 0 deletions res/messages/HCLBundle.properties
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ duplicated.output.inspection.display.name=Duplicated output
duplicated.block.property.inspection.display.name=Duplicated property
interpolations.not.allowed.display.name=Interpolations not allowed
missing.module.inspection.display.name=Module is missing
default.property.inspection.display.name=Property set to default value

hil.scope.not.available.in.context.inspection.display.name=Scope not available in context
hil.unknown.resource.type.inspection.display.name=Unknown resource type referenced
Expand Down
12 changes: 8 additions & 4 deletions schemas-extractor/build-all.sh
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,10 @@ if [ ! -f 'providers.list.full' ]; then
exit 1
fi

if [ -z "$GOPATH" ]; then
GOPATH="$(go env GOPATH)" # use the default GOPATH
fi

out="$CUR/schemas"
mkdir -p "$out"
rm -f "$CUR/failure.txt"
Expand Down Expand Up @@ -51,10 +55,10 @@ for p in $(cat "$CUR/providers.list.full"); do
rm -rf generate-schema
mkdir generate-schema
cp -r "$CUR/template/generate-schema.go" generate-schema/generate-schema.go
sed -i -e "s/__FULL_NAME__/$p/g" generate-schema/generate-schema.go
sed -i -e "s/__NAME__/${p:19}/g" generate-schema/generate-schema.go
sed -i -e "s/__REVISION__/$revision/g" generate-schema/generate-schema.go
sed -i -e "s~__OUT__~$out~g" generate-schema/generate-schema.go
sed -i '' -e "s/__FULL_NAME__/$p/g" generate-schema/generate-schema.go
sed -i '' -e "s/__NAME__/${p:19}/g" generate-schema/generate-schema.go
sed -i '' -e "s/__REVISION__/$revision/g" generate-schema/generate-schema.go
sed -i '' -e "s~__OUT__~$out~g" generate-schema/generate-schema.go

#echo "Building $p"
#make
Expand Down
48 changes: 39 additions & 9 deletions schemas-extractor/template/generate-schema.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import (
"fmt"
"os"
"path/filepath"
"reflect"
"time"
)

Expand Down Expand Up @@ -127,8 +126,11 @@ func export(v *schema.Schema) SchemaDefinition {
}

// TODO: Find better solution
if defValue, err := v.DefaultValue(); err == nil && defValue != nil && !reflect.DeepEqual(defValue, v.Default) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I couldn't see what DeepEqual was achieving here, perhaps something important that I misunderstood?

Copy link
Owner

Choose a reason for hiding this comment

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

Can't remember why I've added it here. Does removing changes anything?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using !reflect.DeepEqual(... there are 265 defaults generated. Removing it makes 3134 defaults generated. The diff is too big to compare everything but I think it is correctly finding all default values when removed.

item.Default = exportValue(defValue, fmt.Sprintf("%T", defValue))
if defValue, err := v.DefaultValue(); err == nil && defValue != nil {
defValueAsMap, ok := defValue.(map[string]interface{})
if !ok || len(defValueAsMap) != 0 {
item.Default = exportValue(defValue, fmt.Sprintf("%T", defValue))
}
}
return item
}
Expand All @@ -151,10 +153,12 @@ func exportValue(value interface{}, t string) *SchemaElement {
}
vt, ok := value.(schema.ValueType)
if ok {
return &SchemaElement{Value: shortenType(fmt.Sprintf("%v", vt))}
valStr := shortenType(fmt.Sprintf("%v", vt))
return &SchemaElement{Value: &valStr}
}
// Unknown case
return &SchemaElement{Type: t, Value: fmt.Sprintf("%v", value)}
valStr := fmt.Sprintf("%v", value)
return &SchemaElement{Type: t, Value: &valStr}
}

func Generate(provider *schema.Provider, name string, outputPath string) {
Expand Down Expand Up @@ -190,13 +194,39 @@ func DoGenerate(provider *schema.Provider, providerName string, outputFilePath s

type SchemaElement struct {
// One of "schema.ValueType" or "SchemaElements" or "SchemaInfo"
Type string `json:",omitempty"`
Type string
// Set for simple types (from ValueType)
Value string `json:",omitempty"`
Value *string
// Set if Type == "SchemaElements"
ElementsType string `json:",omitempty"`
ElementsType string
// Set if Type == "SchemaInfo"
Info SchemaInfo `json:",omitempty"`
Info SchemaInfo
}

func (e *SchemaElement) MarshalJSON() ([]byte, error) {
if e.Value == nil {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe there is a better way? I don't know much about Go.

Copy link
Owner

Choose a reason for hiding this comment

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

Neither do I :)

Choose a reason for hiding this comment

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

Origin

Value string `json:",omitempty"`

Should do the trick. You don't need to specify separate Marshaller for this.

And string in go is actually []byte which is actually already pointer type. And in most cases it is better to use just string instead of *string. And compare string with empty string ("") instead of null. What is literally the same.

I don't understand why original omitempty doesn't work.

return json.Marshal(&struct {
Type string `json:",omitempty"`
ElementsType string `json:",omitempty"`
Info SchemaInfo `json:",omitempty"`
}{
Type: e.Type,
ElementsType: e.ElementsType,
Info: e.Info,
})
} else {
return json.Marshal(&struct {
Type string `json:",omitempty"`
Value string
ElementsType string `json:",omitempty"`
Info SchemaInfo `json:",omitempty"`
}{
Type: e.Type,
Value: *e.Value,
ElementsType: e.ElementsType,
Info: e.Info,
})
}
}

type SchemaDefinition struct {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ class HCLBlockMissingPropertyInspection : LocalInspectionTool() {
val obj = block.`object` ?: return
ProgressIndicatorProvider.checkCanceled()

val candidates = ArrayList<PropertyOrBlockType>(properties.filter { it.required && !(it is PropertyType && it.has_default) })
val candidates = ArrayList<PropertyOrBlockType>(properties.filter { it.required && it.defaultValue == null })
if (candidates.isEmpty()) return
val all = ArrayList<String>()
all.addAll(obj.propertyList.map { it.name })
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
/*
* Copyright 2000-2017 JetBrains s.r.o.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package org.intellij.plugins.hcl.terraform.config.inspection

import com.intellij.codeInsight.intention.LowPriorityAction
import com.intellij.codeInspection.*
import com.intellij.openapi.application.ApplicationManager
import com.intellij.openapi.project.Project
import com.intellij.psi.PsiElementVisitor
import org.intellij.plugins.hcl.psi.*
import org.intellij.plugins.hcl.terraform.config.TerraformFileType
import org.intellij.plugins.hcl.terraform.config.codeinsight.ModelHelper

class TFDefaultPropertyInspection : LocalInspectionTool() {

override fun buildVisitor(holder: ProblemsHolder, isOnTheFly: Boolean): PsiElementVisitor {
return if (holder.file.fileType == TerraformFileType) MyEV(holder) else super.buildVisitor(holder, isOnTheFly)
}

inner class MyEV(val holder: ProblemsHolder) : HCLElementVisitor() {
override fun visitBlock(block: HCLBlock) {
val properties = block.`object`?.propertyList ?: return
val model = ModelHelper.getBlockProperties(block).associateBy { it.name }
for (p in properties) {
val name = p.name
val defaultValue = model[name]?.defaultValue ?: continue
val value = p.value ?: continue
if (isEquals(value, defaultValue)) {
holder.registerProblem(p, "'$name' is set to its default value", ProblemHighlightType.LIKE_UNUSED_SYMBOL, DeletePropertyFix)
}
}
}

private fun isEquals(value: HCLValue, defaultValue: Any): Boolean {
return when (value) {
is HCLBooleanLiteral -> value.value == defaultValue
is HCLNumberLiteral -> value.value == defaultValue
is HCLStringLiteral -> value.value == defaultValue
else -> false
}
}
}

private object DeletePropertyFix : LocalQuickFixBase("Delete property"), LowPriorityAction {
override fun applyFix(project: Project, descriptor: ProblemDescriptor) {
val property = descriptor.psiElement as? HCLProperty ?: return
ApplicationManager.getApplication().runWriteAction { property.delete() }
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
package org.intellij.plugins.hcl.terraform.config.model

import com.beust.klaxon.*
import com.intellij.openapi.application.Application
import com.intellij.openapi.application.ApplicationManager
import com.intellij.openapi.diagnostic.Logger
import com.intellij.openapi.util.SystemInfo
Expand All @@ -35,9 +34,9 @@ class TypeModelLoader(val external: Map<String, TypeModelProvider.Additional>) {
val provisioners: MutableList<ProvisionerType> = arrayListOf()
val backends: MutableList<BackendType> = arrayListOf()
val functions: MutableList<Function> = arrayListOf()
private val application = ApplicationManager.getApplication()

fun load(): TypeModel? {
val application = ApplicationManager.getApplication()
try {
val resources: Collection<String> = getAllResourcesToLoad(ModelResourcesPrefix)

Expand All @@ -49,7 +48,7 @@ class TypeModelLoader(val external: Map<String, TypeModelProvider.Additional>) {
continue
}

loadOne(application, file, stream)
loadOne(file, stream)
}

val schemas = getSharedSchemas()
Expand All @@ -58,10 +57,10 @@ class TypeModelLoader(val external: Map<String, TypeModelProvider.Additional>) {
try {
stream = file.inputStream()
} catch(e: Exception) {
logErrorAndFailInInternalMode(application, "Cannot open stream for file '${file.absolutePath}'", e)
logErrorAndFailInInternalMode("Cannot open stream for file '${file.absolutePath}'", e)
continue
}
loadOne(application, file.absolutePath, stream)
loadOne(file.absolutePath, stream)
}

this.resources.sortBy { it.type }
Expand All @@ -81,35 +80,35 @@ class TypeModelLoader(val external: Map<String, TypeModelProvider.Additional>) {
this.functions.associateBy { it.name }
)
} catch(e: Exception) {
logErrorAndFailInInternalMode(application, "Failed to load Terraform Model", e)
logErrorAndFailInInternalMode("Failed to load Terraform Model", e)
return null
}
}

private fun loadOne(application: Application, file: String, stream: InputStream) {
private fun loadOne(file: String, stream: InputStream) {
val json: JsonObject?
try {
json = stream.use {
val parser = Parser()
parser.parse(stream) as JsonObject?
}
if (json == null) {
logErrorAndFailInInternalMode(application, "In file '$file' no JSON found")
logErrorAndFailInInternalMode("In file '$file' no JSON found")
return
}
} catch(e: Exception) {
logErrorAndFailInInternalMode(application, "Failed to load json data from file '$file'", e)
logErrorAndFailInInternalMode("Failed to load json data from file '$file'", e)
return
}
try {
parseFile(json, file)
} catch(e: Throwable) {
logErrorAndFailInInternalMode(application, "Failed to parse file '$file'", e)
logErrorAndFailInInternalMode("Failed to parse file '$file'", e)
}
return
}

private fun logErrorAndFailInInternalMode(application: Application, msg: String, e: Throwable? = null) {
private fun logErrorAndFailInInternalMode(msg: String, e: Throwable? = null) {
val msg2 = if (e == null) msg else "$msg: ${e.message}"
if (e == null) LOG.error(msg2) else LOG.error(msg2, e)
if (application.isInternal) {
Expand Down Expand Up @@ -322,7 +321,8 @@ class TypeModelLoader(val external: Map<String, TypeModelProvider.Additional>) {
throw IllegalStateException(Constants.TIMEOUTS + " not expected here")
}

val type = parseType(value.string("Type"))
val typeString = value.string("Type")
val type = parseType(typeString)
val elem = value.obj("Elem")
if (elem != null && elem.isNotEmpty()) {
// Valid only for TypeSet and TypeList, should parse internal structure
Expand Down Expand Up @@ -361,7 +361,17 @@ class TypeModelLoader(val external: Map<String, TypeModelProvider.Additional>) {
val conflicts: List<String>? = value.array<String>("ConflictsWith")?.map { it.pool() }

val deprecated = value.string("Deprecated")
val has_default: Boolean = value.obj("Default")?.isNotEmpty() ?: false
val defaultValue: Any? = value.obj("Default")?.string("Value")?.let {
when (typeString) {
"Bool", "TypeBool" -> it.toBoolean()
"Int", "TypeInt" -> it.toInt()
"Float", "TypeFloat", "String", "TypeString" -> it
else -> {
logErrorAndFailInInternalMode("Unhandled default type $typeString for $fqn")
it
}
}
}
// || m["InputDefault"]?.string("value") != null // Not sure about this property TODO: Investigate how it works in terraform

val additional = external[fqn] ?: TypeModelProvider.Additional(name)
Expand Down Expand Up @@ -396,7 +406,7 @@ class TypeModelLoader(val external: Map<String, TypeModelProvider.Additional>) {
deprecated = deprecated?.pool(),
computed = computed,
conflictsWith = conflicts,
has_default = has_default).pool()
defaultValue = defaultValue).pool()
}

private fun parseType(string: String?): Type {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ open class PropertyType(override val name: String, val type: Type,
description: String? = null,
required: Boolean = false, deprecated: String? = null, computed: Boolean = false,
conflictsWith: List<String>? = null,
val has_default: Boolean = false
override val defaultValue: Any? = null
) : BaseModelType(description = description, required = required, deprecated = deprecated, computed = computed, conflictsWith = conflictsWith), PropertyOrBlockType {

override fun toString(): String {
Expand All @@ -92,7 +92,7 @@ open class PropertyType(override val name: String, val type: Type,
if (type != other.type) return false
if (hint != other.hint) return false
if (injectionAllowed != other.injectionAllowed) return false
if (has_default != other.has_default) return false
if (defaultValue != other.defaultValue) return false

return true
}
Expand All @@ -103,7 +103,7 @@ open class PropertyType(override val name: String, val type: Type,
result = 31 * result + type.hashCode()
result = 31 * result + (hint?.hashCode() ?: 0)
result = 31 * result + injectionAllowed.hashCode()
result = 31 * result + has_default.hashCode()
result = 31 * result + (defaultValue?.hashCode() ?: 0)
return result
}

Expand All @@ -124,6 +124,9 @@ open class BlockType(val literal: String, val args: Int = 0,
return "BlockType(literal='$literal', args=$args, properties=${Arrays.toString(properties)})"
}

override val defaultValue: Any?
get() = null

override fun equals(other: Any?): Boolean {
if (this === other) return true
if (javaClass != other?.javaClass) return false
Expand Down Expand Up @@ -158,6 +161,7 @@ interface PropertyOrBlockType {
val deprecated: String?
val computed: Boolean
val conflictsWith: List<String>?
val defaultValue: Any?
}

object Types {
Expand Down