From e3bd33d780631ee59246db750549d2b5aa3cb5e9 Mon Sep 17 00:00:00 2001
From: Dominik Richter <dominik.richter@gmail.com>
Date: Sat, 2 Mar 2024 15:36:22 -0800
Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20avoid=20accidental=20empty=20pro?=
 =?UTF-8?q?perties=20(#3465)?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

A friend accidentally the `query` (which we used pre v9) attribute for
the property, instead of the `mql` field. This led to an empty property,
which in this case is never valid. Properties must be non-empty.

Guarantee non-empty props + add more testing.

Signed-off-by: Dominik Richter <dominik.richter@gmail.com>
---
 explorer/property.go      | 20 +++++++++++++++----
 explorer/property_test.go | 41 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 57 insertions(+), 4 deletions(-)

diff --git a/explorer/property.go b/explorer/property.go
index 1f3ffb6ff9..04c896fe1b 100644
--- a/explorer/property.go
+++ b/explorer/property.go
@@ -45,15 +45,27 @@ func (p *Property) Compile(props map[string]*llx.Primitive, conf mqlc.CompilerCo
 	return mqlc.Compile(p.Mql, props, conf)
 }
 
+// id gets any valid ID for the property, prioritizing uid > mrn > title
+func (p *Property) id() string {
+	if p.Uid != "" {
+		return p.Uid
+	}
+	if p.Mrn != "" {
+		return p.Mrn
+	}
+	// last resort
+	return p.Title
+}
+
 // RefreshChecksumAndType by compiling the query and updating the Checksum field
 func (p *Property) RefreshChecksumAndType(conf mqlc.CompilerConfig) (*llx.CodeBundle, error) {
-	return p.refreshChecksumAndType(conf)
-}
+	if p.Mql == "" {
+		return nil, errors.New("property must not be empty (property '" + p.id() + "')")
+	}
 
-func (p *Property) refreshChecksumAndType(conf mqlc.CompilerConfig) (*llx.CodeBundle, error) {
 	bundle, err := p.Compile(nil, conf)
 	if err != nil {
-		return bundle, multierr.Wrap(err, "failed to compile property '"+p.Mql+"'")
+		return bundle, multierr.Wrap(err, "failed to compile property '"+p.id()+"', mql: '"+p.Mql+"'")
 	}
 
 	if bundle.GetCodeV2().GetId() == "" {
diff --git a/explorer/property_test.go b/explorer/property_test.go
index 9fb3a80c82..18a5058457 100644
--- a/explorer/property_test.go
+++ b/explorer/property_test.go
@@ -8,6 +8,9 @@ import (
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	"go.mondoo.com/cnquery/v10"
+	"go.mondoo.com/cnquery/v10/mqlc"
+	"go.mondoo.com/cnquery/v10/providers"
 )
 
 func TestProperty_RefreshMrn(t *testing.T) {
@@ -26,3 +29,41 @@ func TestProperty_RefreshMrn(t *testing.T) {
 	assert.Equal(t, "", in.For[0].Uid)
 	assert.Equal(t, "//my.owner/queries/uid2", in.For[0].Mrn)
 }
+
+func TestProperty_MustNotBeEmpty(t *testing.T) {
+	in := &Property{
+		Uid: "uid1",
+		Mql: "",
+	}
+
+	schema := providers.DefaultRuntime().Schema()
+	conf := mqlc.NewConfig(schema, cnquery.DefaultFeatures)
+	_, err := in.RefreshChecksumAndType(conf)
+
+	assert.Error(t, err)
+}
+
+func TestProperty_MustCompile(t *testing.T) {
+	schema := providers.DefaultRuntime().Schema()
+	conf := mqlc.NewConfig(schema, cnquery.DefaultFeatures)
+
+	t.Run("fails on invalid MQL", func(t *testing.T) {
+		in := &Property{
+			Uid: "uid1",
+			Mql: "ruri.ryu",
+		}
+
+		_, err := in.RefreshChecksumAndType(conf)
+		assert.Error(t, err)
+	})
+
+	t.Run("works with a valid query", func(t *testing.T) {
+		in := &Property{
+			Uid: "uid1",
+			Mql: "mondoo.version",
+		}
+
+		_, err := in.RefreshChecksumAndType(conf)
+		assert.NoError(t, err)
+	})
+}