Refactor Testcontainers::Network to make it more Ruby idiomatic

Author: guilleiguaran

core/lib/testcontainers/docker_container.rb

@@ -1271,7 +1271,7 @@ def normalize_aliases(aliases)
     def resolve_network(network)
       case network
       when Testcontainers::Network
-        network.create
+        network.create!
         [network.name, network]
       when Docker::Network
         info = network.info || {}

core/lib/testcontainers/network.rb

@@ -1,111 +1,220 @@
 # frozen_string_literal: true
 
 require "securerandom"
+require "singleton"
+require "forwardable"
+
 module Testcontainers
+  # Custom error classes for network operations
+  class NetworkError < StandardError; end
+  class NetworkNotFoundError < NetworkError; end
+  class NetworkAlreadyExistsError < NetworkError; end
+  class NetworkInUseError < NetworkError; end
+
   # Lightweight wrapper for Docker networks with convenience helpers
   class Network
+    extend Forwardable
+    include Enumerable
     DEFAULT_DRIVER = "bridge"
     SHARED_NAME = "testcontainers-shared-network"
 
+    # Delegate methods to the underlying Docker::Network object
+    def_delegators :docker_network, :id, :json
+
     class << self
-      def new_network(name: nil, driver: DEFAULT_DRIVER, options: {})
-        network = build(name: name, driver: driver, options: options)
-        network.create
-        network
+      # Creates and initializes a new Docker network
+      #
+      # @param name [String, nil] Custom network name (auto-generated if nil)
+      # @param driver [String] Network driver (default: "bridge")
+      # @param options [Hash] Additional Docker network options
+      # @yield [Network] Optionally yields the network for block-based resource management
+      # @return [Network] The created network (or result of block if given)
+      #
+      # @example Basic usage
+      #   network = Network.create(name: "my-network")
+      #
+      # @example Block-based usage with automatic cleanup
+      #   Network.create(name: "test-net") do |network|
+      #     container.with_network(network).start
+      #     # Run tests...
+      #   end # network automatically closed
+      def create(name: nil, driver: DEFAULT_DRIVER, options: {})
+        network = new(name: name, driver: driver, options: options)
+        network.create!
+
+        if block_given?
+          begin
+            yield network
+          ensure
+            network.close
+          end
+        else
+          network
+        end
       end
 
+      # Returns the singleton shared network instance
+      #
+      # @return [SharedNetwork] The shared network singleton
       def shared
-        SHARED
+        SharedNetwork.instance
       end
 
+      # Generates a unique network name
+      #
+      # @return [String] A unique network name
       def generate_name
         "testcontainers-network-#{SecureRandom.uuid}"
       end
-
-      private
-
-      def build(name: nil, driver: DEFAULT_DRIVER, options: {}, shared: false)
-        network = new(name: name, driver: driver, options: options)
-        if shared
-          network.instance_variable_set(:@shared, true)
-          network.send(:register_shared_cleanup)
-        end
-        network
-      end
     end
 
     attr_reader :name, :driver, :options
 
     def initialize(name: nil, driver: DEFAULT_DRIVER, options: {})
-      @shared = false
-      @name = name || default_name
+      @name = name || self.class.generate_name
       @driver = driver
       @options = options
       @mutex = Mutex.new
       @docker_network = nil
     end
 
-    def create
+    # Creates the Docker network (idempotent)
+    #
+    # @return [self] Returns self for method chaining
+    # @raise [NetworkAlreadyExistsError] if network with same name already exists
+    def create!
       @mutex.synchronize do
-        return @docker_network if @docker_network
-
-        payload = {"Driver" => @driver, "CheckDuplicate" => true}
-        payload["Options"] = @options if @options && !@options.empty?
-        connection = Testcontainers::DockerClient.connection
-        @docker_network = Docker::Network.create(@name, payload, connection)
+        @docker_network ||= begin
+          payload = {
+            "Driver" => @driver,
+            "CheckDuplicate" => true,
+            "Options" => @options.empty? ? nil : @options
+          }.compact
+
+          connection = Testcontainers::DockerClient.connection
+          Docker::Network.create(@name, payload, connection)
+        rescue Docker::Error::ConflictError => e
+          raise NetworkAlreadyExistsError, "Network '#{@name}' already exists: #{e.message}"
+        end
       end
+
+      self
     end
 
+    # Returns the underlying Docker::Network object, creating it if necessary
+    #
+    # @return [Docker::Network] The Docker network object
     def docker_network
-      create unless @docker_network
-      @docker_network
+      @mutex.synchronize do
+        @docker_network || create!.instance_variable_get(:@docker_network)
+      end
     end
 
+    # Checks if the network has been created
+    #
+    # @return [Boolean] true if network is created, false otherwise
     def created?
-      !!@docker_network
+      !@docker_network.nil?
     end
 
+    # Returns network information from Docker
+    #
+    # @return [Hash] Network information
     def info
       docker_network.json
     end
 
+    # Iterates over containers connected to this network
+    #
+    # @yield [Hash] Container information for each connected container
+    # @return [Enumerator] if no block is given
+    #
+    # @example
+    #   network.each { |container| puts container["Name"] }
+    #   network.map { |c| c["IPv4Address"] }
+    def each(&block)
+      return to_enum(:each) unless block_given?
+
+      containers.each(&block)
+    end
+
+    # Returns containers connected to this network
+    #
+    # @return [Array<Hash>] Array of container information hashes
+    def containers
+      info.dig("Containers")&.values || []
+    end
+
+    # Closes and removes the network (idempotent)
+    #
+    # @param force [Boolean] If true, forcefully removes the network
+    # @return [self] Returns self for method chaining
+    # @raise [NetworkInUseError] if network is in use and force is false
     def close(force: false)
       return self if shared? && !force
 
       @mutex.synchronize do
-        return unless @docker_network
-
-        begin
-          force ? @docker_network.delete : @docker_network.remove
-        rescue Docker::Error::NotFoundError
-          # Swallow missing network errors so cleanup stays idempotent
-        ensure
-          @docker_network = nil
+        @docker_network&.tap do |net|
+          begin
+            removal_method = force ? :delete : :remove
+            net.public_send(removal_method)
+          rescue Docker::Error::NotFoundError
+            # Swallow missing network errors so cleanup stays idempotent
+          rescue Docker::Error::ConflictError, Excon::Error::Forbidden => e
+            raise NetworkInUseError, "Network '#{@name}' is in use: #{e.message}"
+          end
         end
+      ensure
+        @docker_network = nil
       end
+
+      self
     end
 
+    # Forcefully closes and removes the network
+    #
+    # @return [self] Returns self for method chaining
     def force_close
       close(force: true)
     end
 
+    # Checks if this is the shared singleton network
+    #
+    # @return [Boolean] true if this is the shared network
     def shared?
-      @shared
+      false
     end
 
-    private
+    # Alias for close (more explicit naming)
+    alias_method :destroy, :close
+    alias_method :remove, :close
+  end
+
+  # Singleton shared network for multi-container test scenarios
+  #
+  # @example Using the shared network
+  #   shared = Network.shared
+  #   container1.with_network(shared, aliases: ["service1"])
+  #   container2.with_network(shared, aliases: ["service2"])
+  class SharedNetwork < Network
+    include Singleton
+
+    def initialize
+      super(name: SHARED_NAME)
+      register_cleanup
+    end
 
-    def default_name
-      shared? ? SHARED_NAME : self.class.generate_name
+    def shared?
+      true
     end
 
-    def register_shared_cleanup
-      return if self.class.instance_variable_get(:@shared_cleanup_registered)
+    private
 
+    def register_cleanup
       at_exit { force_close }
-      self.class.instance_variable_set(:@shared_cleanup_registered, true)
     end
   end
 
-  Network::SHARED = Network.__send__(:build, name: Network::SHARED_NAME, shared: true)
+  # Backward compatibility: SHARED constant points to the singleton instance
+  Network::SHARED = SharedNetwork.instance
 end

core/test/docker_container_test.rb

@@ -79,7 +79,7 @@ def test_it_uses_locally_built_image_before_pulling
   end
 
   def test_it_attaches_to_custom_network_with_aliases
-    network = Testcontainers::Network.new_network
+    network = Testcontainers::Network.create
     container = Testcontainers::DockerContainer.new("alpine:latest", command: %w[sleep 60])
       .with_network(network, aliases: ["web"])
 
@@ -96,7 +96,7 @@ def test_it_attaches_to_custom_network_with_aliases
   end
 
   def test_it_applies_pending_aliases_before_network_assignment
-    network = Testcontainers::Network.new_network
+    network = Testcontainers::Network.create
     container = Testcontainers::DockerContainer.new("alpine:latest", command: %w[sleep 60])
     container.with_network_aliases("app")
     container.with_network(network)
@@ -112,8 +112,8 @@ def test_it_applies_pending_aliases_before_network_assignment
   end
 
   def test_it_attaches_to_multiple_networks
-    primary_network = Testcontainers::Network.new_network
-    secondary_network = Testcontainers::Network.new_network
+    primary_network = Testcontainers::Network.create
+    secondary_network = Testcontainers::Network.create
     container = Testcontainers::DockerContainer
       .new("alpine:latest", command: %w[sleep 60])
       .with_network(primary_network, aliases: ["primary"])