[rego] Check store modules before skipping parsing (#5520)

* [rego] Check store modules before skipping parsing

Fixes #5511

This change will cause the operation to be timed if the store
modules have already all been compiled and there are no
rawModules. This might be undesirable.

Signed-off-by: Charlie Egan <charlie@styra.com>

plugins/logs/plugin_test.go

@@ -1705,6 +1705,7 @@ func TestPluginMasking(t *testing.T) {
 					if tc.errManager.Error() != err.Error() {
 						t.Fatalf("expected error %s, but got %s", tc.errManager.Error(), err.Error())
 					}
+					return
 				}
 			}
 

rego/rego.go

@@ -1689,7 +1689,14 @@ func (r *Rego) prepare(ctx context.Context, qType queryType, extras []extraStage
 }
 
 func (r *Rego) parseModules(ctx context.Context, txn storage.Transaction, m metrics.Metrics) error {
-	if len(r.modules) == 0 {
+	ids, err := r.store.ListPolicies(ctx, txn)
+	if err != nil {
+		return err
+	}
+
+	// if there are no raw modules, nor modules in the store, then there
+	// is nothing to do.
+	if len(r.modules) == 0 && len(ids) == 0 {
 		return nil
 	}
 
@@ -1700,11 +1707,6 @@ func (r *Rego) parseModules(ctx context.Context, txn storage.Transaction, m metr
 	// Parse any modules in the are saved to the store, but only if
 	// another compile step is going to occur (ie. we have parsed modules
 	// that need to be compiled).
-	ids, err := r.store.ListPolicies(ctx, txn)
-	if err != nil {
-		return err
-	}
-
 	for _, id := range ids {
 		// if it is already on the compiler we're using
 		// then don't bother to re-parse it from source

sdk/opa_test.go

@@ -408,7 +408,7 @@ main = true
 		t.Fatal("expected true but got:", decision, ok)
 	}
 
-	if exp, act := 4, len(m.All()); exp != act {
+	if exp, act := 5, len(m.All()); exp != act {
 		t.Fatalf("expected %d metrics, got %d", exp, act)
 	}
 
@@ -484,7 +484,7 @@ main = true
 		t.Fatal("expected true but got:", decision, ok)
 	}
 
-	if exp, act := 25, len(m.All()); exp != act {
+	if exp, act := 26, len(m.All()); exp != act {
 		t.Fatalf("expected %d metrics, got %d", exp, act)
 	}
 
@@ -932,7 +932,7 @@ allow {
 		t.Fatal("expected &{[2 = data.junk.x] []} true but got:", decision, ok)
 	}
 
-	if exp, act := 5, len(m.All()); exp != act {
+	if exp, act := 6, len(m.All()); exp != act {
 		t.Fatalf("expected %d metrics, got %d", exp, act)
 	}
 
@@ -1019,7 +1019,7 @@ allow {
 		t.Fatal("expected &{[2 = data.junk.x] []} true but got:", decision, ok)
 	}
 
-	if exp, act := 32, len(m.All()); exp != act {
+	if exp, act := 33, len(m.All()); exp != act {
 		t.Fatalf("expected %d metrics, got %d", exp, act)
 	}
 

server/server_test.go

@@ -1034,6 +1034,7 @@ func TestCompileV1Observability(t *testing.T) {
 			"timer_rego_partial_eval_ns",
 			"timer_rego_query_compile_ns",
 			"timer_rego_query_parse_ns",
+			"timer_rego_module_parse_ns",
 			"timer_server_handler_ns",
 			"counter_disk_read_keys",
 			"timer_disk_read_ns",