From fbb68dbcd0c3eae6ee82c3253bc91e7f425819e6 Mon Sep 17 00:00:00 2001 From: Julian Brost Date: Wed, 7 Dec 2022 11:18:28 +0100 Subject: [PATCH] Namespace: replace behavior classes with a bool In essence, namespace behaviors acted as hooks for update operations on namespaces. Two behaviors were implemented: - `NamespaceBehavior`: allows the update operation unless it acts on a value that itself was explicitly marked as constant. - `ConstNamespaceBehavior`: initially allows insert operations but marks the individual values as const. Additionally provides a `Freeze()` member function. After this was called, updates are rejected unless a special `overrideFrozen` flag is set explicitly. This marvel of object-oriented programming can be replaced with a simple bool. This commit basically replaces `Namespace::m_Behavior` with `Namespace::m_ConstValues` and inlines the behavior functions where they were called. While doing so, the code was slightly simplified by assuming that `m_ConstValues` is true if `m_Frozen` is true. This is similar to what the API allowed in the old code as you could only freeze a `ConstNamespaceBehavior`. However, this PR moves the `Freeze()` member function and the related `m_Freeze` member variable to the `Namespace` class. So now the API allows any namespace to be frozen. The new code also makes sense with the previously mentioned simplification: a `Namespace` with `m_ConstValues = false` can be modified without restrictions until `Freeze()` is called. When this is done, it becomes read-only. The changes outside of `namespace.*` just adapt the code to the slightly changed API. --- lib/base/json-script.cpp | 5 +- lib/base/math-script.cpp | 5 +- lib/base/namespace.cpp | 88 ++++++++++++++++---------------- lib/base/namespace.hpp | 24 ++------- lib/base/scriptframe.cpp | 23 ++++----- lib/config/expression.cpp | 2 +- lib/icinga/icingaapplication.cpp | 5 +- 7 files changed, 65 insertions(+), 87 deletions(-) diff --git a/lib/base/json-script.cpp b/lib/base/json-script.cpp index 54d663177..cb3c03392 100644 --- a/lib/base/json-script.cpp +++ b/lib/base/json-script.cpp @@ -15,14 +15,13 @@ static String JsonEncodeShim(const Value& value) } INITIALIZE_ONCE([]() { - auto jsonNSBehavior = new ConstNamespaceBehavior(); - Namespace::Ptr jsonNS = new Namespace(jsonNSBehavior); + Namespace::Ptr jsonNS = new Namespace(true); /* Methods */ jsonNS->Set("encode", new Function("Json#encode", JsonEncodeShim, { "value" }, true)); jsonNS->Set("decode", new Function("Json#decode", JsonDecode, { "value" }, true)); - jsonNSBehavior->Freeze(); + jsonNS->Freeze(); Namespace::Ptr systemNS = ScriptGlobal::Get("System"); systemNS->SetAttribute("Json", new ConstEmbeddedNamespaceValue(jsonNS)); diff --git a/lib/base/math-script.cpp b/lib/base/math-script.cpp index 0060513c2..dbc2fb285 100644 --- a/lib/base/math-script.cpp +++ b/lib/base/math-script.cpp @@ -142,8 +142,7 @@ static double MathSign(double x) } INITIALIZE_ONCE([]() { - auto mathNSBehavior = new ConstNamespaceBehavior(); - Namespace::Ptr mathNS = new Namespace(mathNSBehavior); + Namespace::Ptr mathNS = new Namespace(true); /* Constants */ mathNS->Set("E", 2.71828182845904523536); @@ -178,7 +177,7 @@ INITIALIZE_ONCE([]() { mathNS->Set("isinf", new Function("Math#isinf", MathIsinf, { "x" }, true)); mathNS->Set("sign", new Function("Math#sign", MathSign, { "x" }, true)); - mathNSBehavior->Freeze(); + mathNS->Freeze(); Namespace::Ptr systemNS = ScriptGlobal::Get("System"); systemNS->SetAttribute("Math", new ConstEmbeddedNamespaceValue(mathNS)); diff --git a/lib/base/namespace.cpp b/lib/base/namespace.cpp index 001fa33d1..d9947f1f3 100644 --- a/lib/base/namespace.cpp +++ b/lib/base/namespace.cpp @@ -14,8 +14,13 @@ template class std::map REGISTER_PRIMITIVE_TYPE(Namespace, Object, Namespace::GetPrototype()); -Namespace::Namespace(NamespaceBehavior *behavior) - : m_Behavior(std::unique_ptr(behavior)) +/** + * Creates a new namespace. + * + * @param constValues If true, all values inserted into the namespace are treated as constants and can't be updated. + */ +Namespace::Namespace(bool constValues) + : m_ConstValues(constValues), m_Frozen(false) { } Value Namespace::Get(const String& field) const @@ -71,7 +76,31 @@ void Namespace::Remove(const String& field, bool overrideFrozen) { ObjectLock olock(this); - m_Behavior->Remove(this, field, overrideFrozen); + if (m_Frozen && !overrideFrozen) { + BOOST_THROW_EXCEPTION(ScriptError("Namespace is read-only and must not be modified.")); + } + + if (!overrideFrozen) { + auto attr = GetAttribute(field); + + if (dynamic_pointer_cast(attr)) { + BOOST_THROW_EXCEPTION(ScriptError("Constants must not be removed.")); + } + } + + RemoveAttribute(field); +} + +/** + * Freeze the namespace, preventing further updates unless overrideFrozen is set. + * + * This only prevents inserting, replacing or deleting values from the namespace. This operation has no effect on + * objects referenced by the values, these remain mutable if they were before. + */ +void Namespace::Freeze() { + ObjectLock olock(this); + + m_Frozen = true; } void Namespace::RemoveAttribute(const String& field) @@ -124,10 +153,19 @@ void Namespace::SetFieldByName(const String& field, const Value& value, bool ove auto nsVal = GetAttribute(field); - if (!nsVal) - m_Behavior->Register(this, field, value, overrideFrozen, debugInfo); - else + if (!nsVal) { + if (m_Frozen && !overrideFrozen) { + BOOST_THROW_EXCEPTION(ScriptError("Namespace is read-only and must not be modified.", debugInfo)); + } + + if (m_ConstValues) { + SetAttribute(field, new ConstEmbeddedNamespaceValue(value)); + } else { + SetAttribute(field, new EmbeddedNamespaceValue(value)); + } + } else { nsVal->Set(value, overrideFrozen, debugInfo); + } } bool Namespace::HasOwnField(const String& field) const @@ -172,44 +210,6 @@ void ConstEmbeddedNamespaceValue::Set(const Value& value, bool overrideFrozen, c EmbeddedNamespaceValue::Set(value, overrideFrozen, debugInfo); } -void NamespaceBehavior::Register(const Namespace::Ptr& ns, const String& field, const Value& value, bool overrideFrozen, const DebugInfo& debugInfo) const -{ - ns->SetAttribute(field, new EmbeddedNamespaceValue(value)); -} - -void NamespaceBehavior::Remove(const Namespace::Ptr& ns, const String& field, bool overrideFrozen) -{ - if (!overrideFrozen) { - auto attr = ns->GetAttribute(field); - - if (dynamic_pointer_cast(attr)) - BOOST_THROW_EXCEPTION(ScriptError("Constants must not be removed.")); - } - - ns->RemoveAttribute(field); -} - -void ConstNamespaceBehavior::Register(const Namespace::Ptr& ns, const String& field, const Value& value, bool overrideFrozen, const DebugInfo& debugInfo) const -{ - if (m_Frozen && !overrideFrozen) - BOOST_THROW_EXCEPTION(ScriptError("Namespace is read-only and must not be modified.", debugInfo)); - - ns->SetAttribute(field, new ConstEmbeddedNamespaceValue(value)); -} - -void ConstNamespaceBehavior::Remove(const Namespace::Ptr& ns, const String& field, bool overrideFrozen) -{ - if (m_Frozen && !overrideFrozen) - BOOST_THROW_EXCEPTION(ScriptError("Namespace is read-only and must not be modified.")); - - NamespaceBehavior::Remove(ns, field, overrideFrozen); -} - -void ConstNamespaceBehavior::Freeze() -{ - m_Frozen = true; -} - Namespace::Iterator Namespace::Begin() { ASSERT(OwnsLock()); diff --git a/lib/base/namespace.hpp b/lib/base/namespace.hpp index 6a53cb17d..492d62f0e 100644 --- a/lib/base/namespace.hpp +++ b/lib/base/namespace.hpp @@ -41,24 +41,6 @@ struct ConstEmbeddedNamespaceValue : public EmbeddedNamespaceValue void Set(const Value& value, bool overrideFrozen, const DebugInfo& debugInfo) override; }; -class Namespace; - -struct NamespaceBehavior -{ - virtual void Register(const boost::intrusive_ptr& ns, const String& field, const Value& value, bool overrideFrozen, const DebugInfo& debugInfo) const; - virtual void Remove(const boost::intrusive_ptr& ns, const String& field, bool overrideFrozen); -}; - -struct ConstNamespaceBehavior : public NamespaceBehavior -{ - void Register(const boost::intrusive_ptr& ns, const String& field, const Value& value, bool overrideFrozen, const DebugInfo& debugInfo) const override; - void Remove(const boost::intrusive_ptr& ns, const String& field, bool overrideFrozen) override; - void Freeze(); - -private: - bool m_Frozen; -}; - /** * A namespace. * @@ -73,13 +55,14 @@ public: typedef std::map::value_type Pair; - Namespace(NamespaceBehavior *behavior = new NamespaceBehavior); + explicit Namespace(bool constValues = false); Value Get(const String& field) const; bool Get(const String& field, Value *value) const; void Set(const String& field, const Value& value, bool overrideFrozen = false); bool Contains(const String& field) const; void Remove(const String& field, bool overrideFrozen = false); + void Freeze(); NamespaceValue::Ptr GetAttribute(const String& field) const; void SetAttribute(const String& field, const NamespaceValue::Ptr& nsVal); @@ -99,7 +82,8 @@ public: private: std::map m_Data; - std::unique_ptr m_Behavior; + bool m_ConstValues; + bool m_Frozen; }; Namespace::Iterator begin(const Namespace::Ptr& x); diff --git a/lib/base/scriptframe.cpp b/lib/base/scriptframe.cpp index 8369e55e0..2001e8c0b 100644 --- a/lib/base/scriptframe.cpp +++ b/lib/base/scriptframe.cpp @@ -10,7 +10,7 @@ using namespace icinga; boost::thread_specific_ptr > ScriptFrame::m_ScriptFrames; -static auto l_InternalNSBehavior = new ConstNamespaceBehavior(); +static Namespace::Ptr l_InternalNS; /* Ensure that this gets called with highest priority * and wins against other static initializers in lib/icinga, etc. @@ -19,29 +19,26 @@ static auto l_InternalNSBehavior = new ConstNamespaceBehavior(); INITIALIZE_ONCE_WITH_PRIORITY([]() { Namespace::Ptr globalNS = ScriptGlobal::GetGlobals(); - auto systemNSBehavior = new ConstNamespaceBehavior(); - systemNSBehavior->Freeze(); - Namespace::Ptr systemNS = new Namespace(systemNSBehavior); + Namespace::Ptr systemNS = new Namespace(true); + systemNS->Freeze(); globalNS->SetAttribute("System", new ConstEmbeddedNamespaceValue(systemNS)); systemNS->SetAttribute("Configuration", new EmbeddedNamespaceValue(new Configuration())); - auto typesNSBehavior = new ConstNamespaceBehavior(); - typesNSBehavior->Freeze(); - Namespace::Ptr typesNS = new Namespace(typesNSBehavior); + Namespace::Ptr typesNS = new Namespace(true); + typesNS->Freeze(); globalNS->SetAttribute("Types", new ConstEmbeddedNamespaceValue(typesNS)); - auto statsNSBehavior = new ConstNamespaceBehavior(); - statsNSBehavior->Freeze(); - Namespace::Ptr statsNS = new Namespace(statsNSBehavior); + Namespace::Ptr statsNS = new Namespace(true); + statsNS->Freeze(); globalNS->SetAttribute("StatsFunctions", new ConstEmbeddedNamespaceValue(statsNS)); - Namespace::Ptr internalNS = new Namespace(l_InternalNSBehavior); - globalNS->SetAttribute("Internal", new ConstEmbeddedNamespaceValue(internalNS)); + l_InternalNS = new Namespace(true); + globalNS->SetAttribute("Internal", new ConstEmbeddedNamespaceValue(l_InternalNS)); }, 1000); INITIALIZE_ONCE_WITH_PRIORITY([]() { - l_InternalNSBehavior->Freeze(); + l_InternalNS->Freeze(); }, 0); ScriptFrame::ScriptFrame(bool allocLocals) diff --git a/lib/config/expression.cpp b/lib/config/expression.cpp index 93ce79a80..ba7ff6c3f 100644 --- a/lib/config/expression.cpp +++ b/lib/config/expression.cpp @@ -930,7 +930,7 @@ ExpressionResult ApplyExpression::DoEvaluate(ScriptFrame& frame, DebugHint *dhin ExpressionResult NamespaceExpression::DoEvaluate(ScriptFrame& frame, DebugHint *dhint) const { - Namespace::Ptr ns = new Namespace(new ConstNamespaceBehavior()); + Namespace::Ptr ns = new Namespace(true); ScriptFrame innerFrame(true, ns); ExpressionResult result = m_Expression->Evaluate(innerFrame); diff --git a/lib/icinga/icingaapplication.cpp b/lib/icinga/icingaapplication.cpp index 1ff303dcb..e98f5e757 100644 --- a/lib/icinga/icingaapplication.cpp +++ b/lib/icinga/icingaapplication.cpp @@ -58,9 +58,8 @@ void IcingaApplication::StaticInitialize() Namespace::Ptr globalNS = ScriptGlobal::GetGlobals(); VERIFY(globalNS); - auto icingaNSBehavior = new ConstNamespaceBehavior(); - icingaNSBehavior->Freeze(); - Namespace::Ptr icingaNS = new Namespace(icingaNSBehavior); + Namespace::Ptr icingaNS = new Namespace(true); + icingaNS->Freeze(); globalNS->SetAttribute("Icinga", new ConstEmbeddedNamespaceValue(icingaNS)); }