Merge pull request GafferHQ#5307 from johnhaddon/nameChecks

Name validation

Changes.md

@@ -43,6 +43,8 @@ Fixes
 - Seeds :
  - Fixed duplicate points at triangle edges.
  - Fixed handling of points exactly at the density threshold.
+- ObjectSource, Group : Prevented the creation of locations with invalid names - `..`, `...` or anything containing `/` or a filter wildcard.
+- BranchCreator : Prevented the use of `...` and other filter wildcards in the `destination`.
 
 API
 ---
@@ -64,6 +66,7 @@ API
 - VectorDataWidget : Added `setErrored()` and `getErrored()` methods to control an error state. Errors are reflected by a red background colour.
 - PlugLayout : Added support for `layout:minimumWidth` metadata.
 - Removed use of `RTLD_GLOBAL` for loading Python modules.
+- SceneAlgo : Added `validateName()` function.
 
 Breaking Changes
 ----------------

include/GafferScene/ObjectSource.h

@@ -108,6 +108,7 @@ class GAFFERSCENE_API ObjectSource : public SceneNode
 
  private :
 
+ IECore::InternedString validatedName() const;
  bool setNameValid( const IECore::InternedString &setName ) const;
 
  static size_t g_firstPlugIndex;

include/GafferScene/SceneAlgo.h

@@ -269,6 +269,10 @@ GAFFERSCENE_API bool visible( const ScenePlug *scene, const ScenePlug::ScenePath
 /// for other object types we must return a synthetic bound.
 GAFFERSCENE_API Imath::Box3f bound( const IECore::Object *object );
 
+/// Throws an exception if `name` is not valid to be used as the name of a scene
+/// location.
+GAFFERSCENE_API void validateName( IECore::InternedString name );
+
 /// Render Adaptors
 /// ===============
 

python/GafferSceneTest/GroupTest.py

@@ -795,6 +795,22 @@ def testExists( self ) :
  self.assertFalse( group["out"].exists( "/group/plane" ) )
  self.assertFalse( group["out"].exists( "/road/to/nowhere" ) )
 
+ def testInvalidNames( self ) :
+
+ plane = GafferScene.Plane()
+ plane["sets"].setValue( "A" )
+
+ group = GafferScene.Group()
+ group["in"][0].setInput( plane["out"] )
+
+ for name in [ "a/b", "a/", "/b", "/", "..", "..." ] :
+ with self.subTest( name = name ) :
+ group["name"].setValue( name )
+ with self.assertRaises( Gaffer.ProcessException ) :
+ group["out"].childNames( "/" )
+ with self.assertRaises( Gaffer.ProcessException ) :
+ group["out"].set( "A" )
+
  def setUp( self ) :
 
  GafferSceneTest.SceneTestCase.setUp( self )

python/GafferSceneTest/ParentTest.py

@@ -915,5 +915,27 @@ def testNoUnwantedBoundEvaluations( self ) :
  parent["in"].boundHash( "/GAFFERBOT/C_torso_GRP" )
  )
 
+ def testInvalidDestinationNames( self ) :
+
+ sphere = GafferScene.Sphere()
+
+ sphereFilter = GafferScene.PathFilter()
+ sphereFilter["paths"].setValue( IECore.StringVectorData( [ "/sphere" ] ) )
+
+ cube = GafferScene.Cube()
+
+ parent = GafferScene.Parent()
+ parent["in"].setInput( sphere["out"] )
+ parent["children"][0].setInput( cube["out"] )
+ parent["filter"].setInput( sphereFilter["out"] )
+
+ parent["destination"].setValue( "/woops/..." )
+ with self.assertRaisesRegex( Gaffer.ProcessException, r'.*Invalid destination `/woops/...`. Name `...` is invalid \(because `...` is a filter wildcard\)' ) :
+ parent["out"].childNames( "/" )
+
+ parent["destination"].setValue( "/woops/a*" )
+ with self.assertRaisesRegex( Gaffer.ProcessException, r".*Invalid destination `/woops/a\*`. Name `a\*` is invalid \(because it contains filter wildcards\)" ) :
+ parent["out"].childNames( "/" )
+
 if __name__ == "__main__":
  unittest.main()

python/GafferSceneTest/SceneAlgoTest.py

@@ -1818,11 +1818,21 @@ def a() :
  self.assertEqual( mh.messages[0].context, "SceneAlgo::createRenderAdaptors" )
  self.assertEqual( mh.messages[0].message, "Adaptor \"Test\" returned null" )
 
+ def testValidateName( self ) :
+
+ for goodName in [ "obi", "lewis", "ludo" ] :
+ with self.subTest( name = goodName ) :
+ GafferScene.SceneAlgo.validateName( goodName )
+
+ for badName in [ "..", "...", "", "a/b", "/", "a/", "/b", "*", "a*", "b*", "[", "b[a-z]", "\\", "\\a", "b\\", "a?", "?a" ] :
+ with self.subTest( name = badName ) :
+ with self.assertRaises( RuntimeError ) :
+ GafferScene.SceneAlgo.validateName( badName )
+
  def tearDown( self ) :
 
  GafferSceneTest.SceneTestCase.tearDown( self )
  GafferScene.SceneAlgo.deregisterRenderAdaptor( "Test" )
 
-
 if __name__ == "__main__":
  unittest.main()

python/GafferSceneTest/SphereTest.py

@@ -215,5 +215,31 @@ def testTransformAffectsBound( self ) :
  s = GafferScene.Sphere()
  self.assertTrue( s["out"]["bound"] in s.affects( s["transform"]["translate"]["x"] ) )
 
+ def testInvalidNames( self ) :
+
+ sphere = GafferScene.Sphere()
+ sphere["sets"].setValue( "A" )
+
+ for name in [ "a/b", "a/", "/b", "/", "..", "...", "*", "a*", "b*", "[", "b[a-z]", r"\\" ] :
+ with self.subTest( name = name ) :
+ sphere["name"].setValue( name )
+ with self.assertRaises( Gaffer.ProcessException ) :
+ sphere["out"].childNames( "/" )
+ with self.assertRaises( Gaffer.ProcessException ) :
+ sphere["out"].set( "A" )
+
+ def testEmptyName( self ) :
+
+ # I think it would be better if we errored for empty names,
+ # but while we don't, we should at least check that the name
+ # we make up is used consistently.
+
+ sphere = GafferScene.Sphere()
+ sphere["name"].setValue( "" )
+ sphere["sets"].setValue( "A" )
+
+ self.assertEqual( sphere["out"].childNames( "/" ), IECore.InternedStringVectorData( [ "unnamed" ] ) )
+ self.assertEqual( sphere["out"].set( "A" ).value, IECore.PathMatcher( [ "/unnamed" ] ) )
+
 if __name__ == "__main__":
  unittest.main()

src/GafferScene/BranchCreator.cpp

@@ -108,6 +108,29 @@ bool internedStringValueLess( const InternedString &a, const InternedString &b )
  return a.string() < b.string();
 }
 
+const InternedString g_ellipsis( "..." );
+
+void validateDestination( const ScenePlug::ScenePath &destination )
+{
+ for( auto &n : destination )
+ {
+ try
+ {
+ SceneAlgo::validateName( n );
+ }
+ catch( const std::exception &e )
+ {
+ throw IECore::Exception(
+ fmt::format(
+ "Invalid destination `{}`. {}",
+ ScenePlug::pathToString( destination ),
+ e.what()
+ )
+ );
+ }
+ }
+}
+
 } // namespace
 
 //////////////////////////////////////////////////////////////////////////
@@ -301,7 +324,9 @@ class BranchCreator::BranchesData : public IECore::Data
  h.append( path.data(), path.size() );
  h.append( (uint64_t)path.size() );
 
- ScenePlug::ScenePath destination = ScenePlug::stringToPath( branchCreator->destinationPlug()->getValue() );
+ const ScenePlug::ScenePath destination = ScenePlug::stringToPath( branchCreator->destinationPlug()->getValue() );
+ validateDestination( destination );
+
  h.append( destination.data(), destination.size() );
  h.append( (uint64_t)destination.size() );
 
@@ -312,7 +337,8 @@ class BranchCreator::BranchesData : public IECore::Data
 
  void addBranch( const BranchCreator *branchCreator, const ScenePlug::ScenePath &path )
  {
- ScenePlug::ScenePath destination = ScenePlug::stringToPath( branchCreator->destinationPlug()->getValue() );
+ const ScenePlug::ScenePath destination = ScenePlug::stringToPath( branchCreator->destinationPlug()->getValue() );
+ validateDestination( destination );
  const ScenePlug::ScenePath existing = closestExistingPath( branchCreator->inPlug(), destination );
 
  tbb::spin_mutex::scoped_lock lock( m_mutex );

src/GafferScene/Group.cpp

@@ -38,6 +38,7 @@
 #include "GafferScene/Group.h"
 
 #include "GafferScene/Private/ChildNamesMap.h"
+#include "GafferScene/SceneAlgo.h"
 
 #include "Gaffer/ArrayPlug.h"
 #include "Gaffer/Context.h"
@@ -375,8 +376,10 @@ IECore::ConstInternedStringVectorDataPtr Group::computeChildNames( const ScenePa
 {
  if( path.size() == 0 )
  {
+ const InternedString name = namePlug()->getValue();
+ SceneAlgo::validateName( name );
  InternedStringVectorDataPtr result = new InternedStringVectorData();
- result->writable().push_back( namePlug()->getValue() );
+ result->writable().push_back( name );
  return result;
  }
  else if( path.size() == 1 )
@@ -449,6 +452,7 @@ IECore::ConstPathMatcherDataPtr Group::computeSet( const IECore::InternedString
  ScenePlug::GlobalScope s( context );
  Private::ConstChildNamesMapPtr mapping = boost::static_pointer_cast<const Private::ChildNamesMap>( mappingPlug()->getValue() );
  const InternedString name = namePlug()->getValue();
+ SceneAlgo::validateName( name );
 
  PathMatcherDataPtr resultData = new PathMatcherData;
  resultData->writable().addPaths( mapping->set( inputSets ), { name } );

src/GafferScene/ObjectSource.cpp

@@ -253,15 +253,7 @@ IECore::ConstInternedStringVectorDataPtr ObjectSource::computeChildNames( const
  if( path.size() == 0 )
  {
  IECore::InternedStringVectorDataPtr result = new IECore::InternedStringVectorData();
- const std::string &name = namePlug()->getValue();
- if( name.size() )
- {
- result->writable().push_back( name );
- }
- else
- {
- result->writable().push_back( "unnamed" );
- }
+ result->writable().push_back( validatedName() );
  return result;
  }
  return parent->childNamesPlug()->defaultValue();
@@ -314,7 +306,7 @@ IECore::ConstPathMatcherDataPtr ObjectSource::computeSet( const IECore::Interned
  if( setNameValid( setName ) )
  {
  IECore::PathMatcherDataPtr result = new IECore::PathMatcherData;
- result->writable().addPath( namePlug()->getValue() );
+ result->writable().addPath( ScenePlug::ScenePath( { validatedName() } ) );
  return result;
  }
  else
@@ -329,6 +321,21 @@ IECore::ConstInternedStringVectorDataPtr ObjectSource::computeStandardSetNames()
  return result;
 }
 
+IECore::InternedString ObjectSource::validatedName() const
+{
+ const IECore::InternedString name = namePlug()->getValue();
+ if( name.string().size() )
+ {
+ SceneAlgo::validateName( name );
+ return name;
+ }
+ else
+ {
+ /// \todo Why don't we just let `validateName()` throw for us instead?
+ return "unnamed";
+ }
+}
+
 bool ObjectSource::setNameValid( const IECore::InternedString &setName ) const
 {
  IECore::ConstInternedStringVectorDataPtr standardSets = computeStandardSetNames();

src/GafferScene/SceneAlgo.cpp

@@ -1122,6 +1122,47 @@ Imath::Box3f GafferScene::SceneAlgo::bound( const IECore::Object *object )
  }
 }
 
+namespace
+{
+
+const InternedString g_emptyInternedString;
+const InternedString g_ellipsisInternedString( "..." );
+const InternedString g_parentInternedString( ".." );
+
+} // namespace
+
+void GafferScene::SceneAlgo::validateName( IECore::InternedString name )
+{
+ const char *invalidReason = nullptr;
+ if( name == g_emptyInternedString )
+ {
+ invalidReason = "it is empty";
+ }
+ else if( name == g_ellipsisInternedString )
+ {
+ invalidReason = "`...` is a filter wildcard";
+ }
+ else if( name == g_parentInternedString )
+ {
+ invalidReason = "`..` denotes the parent location";
+ }
+ else if( name.string().find( '/' ) != string::npos )
+ {
+ invalidReason = "`/` is the path separator";
+ }
+ else if( StringAlgo::hasWildcards( name.string() ) )
+ {
+ invalidReason = "it contains filter wildcards";
+ }
+
+ if( invalidReason )
+ {
+ throw IECore::Exception(
+ fmt::format( "Name `{}` is invalid (because {})", name.string(), invalidReason )
+ );
+ }
+}
+
 //////////////////////////////////////////////////////////////////////////
 // Render Adaptor Registry
 //////////////////////////////////////////////////////////////////////////

src/GafferSceneModule/SceneAlgoBinding.cpp

@@ -310,6 +310,7 @@ void bindSceneAlgo()
 
  def( "exists", &existsWrapper );
  def( "visible", visibleWrapper );
+ def( "validateName", &SceneAlgo::validateName );
 
  def( "filteredNodes", &filteredNodesWrapper );
  def( "matchingPaths", &matchingPathsWrapper1 );