SONAR-1378 validate type when creating/updating properties with the web service /api/properties

3 files changed, 56 insertions, 79 deletions

diff --git a/sonar-server/src/main/webapp/WEB-INF/app/controllers/api/properties_controller.rb b/sonar-server/src/main/webapp/WEB-INF/app/controllers/api/properties_controller.rb
index fbb861d4736..cb5586a9524 100644
--- a/sonar-server/src/main/webapp/WEB-INF/app/controllers/api/properties_controller.rb
+++ b/sonar-server/src/main/webapp/WEB-INF/app/controllers/api/properties_controller.rb
@@ -20,123 +20,89 @@
 
 require "json"
 
-class Api::PropertiesController < Api::RestController
+class Api::PropertiesController < Api::ApiController
 
-  before_filter :admin_required, :only => [ :create, :update, :destroy ]
+  before_filter :admin_required, :only => [:create, :update, :destroy]
 
+  # curl http://localhost:9000/api/properties -v
   def index
     properties = Property.find(:all, :conditions => ['resource_id is null and user_id is null']).select do |property|
       viewable?(property.key)
     end
-    rest_render(properties)
+    respond_to do |format|
+      format.json { render :json => jsonp(to_json(properties)) }
+      format.xml { render :xml => to_xml(properties) }
+    end
   end
 
+  # curl http://localhost:9000/api/properties/<key>[?resource=<resource>] -v
   def show
     key = params[:id]
     resource_id_or_key = params[:resource]
     if resource_id_or_key
       resource = Project.by_key(resource_id_or_key)
-      if resource
-        prop = Property.by_key(key, resource.id)
-      else
-        rest_status_ko('Resource [' + resource_id_or_key + '] does not exist', 404)
-        return
-      end
+      not_found('resource not found') unless resource
+      prop = Property.by_key(key, resource.id)
     else
       prop = Property.by_key(key)
     end
-    if prop
-      if viewable?(key)
-        rest_render([prop])
-      else
-        rest_status_ko('You are not authorized to see this ressource', 401)
-      end
-    else
-      rest_status_ko('Property [' + params[:id] + '] does not exist', 404)
+    not_found('property not found') unless prop
+    access_denied unless viewable?(key)
+    respond_to do |format|
+      format.json { render :json => jsonp(to_json([prop])) }
+      format.xml { render :xml => to_xml([prop]) }
     end
   end
 
+  # curl -u admin:admin -v -X PUT http://localhost:9000/api/properties/foo?value=bar[&resource=<resource>]
   def create
-    key = params[:id]
-    value = params[:value] || request.raw_post
-    resource_id_or_key = params[:resource]
-    if resource_id_or_key
-      resource = Project.by_key(resource_id_or_key)
-      if resource
-        resource_id_or_key = resource.id
-      else
-        rest_status_ko('Resource [' + resource_id_or_key + '] does not exist', 404)
-        return
-      end
-    end
-    if key
-      begin
-        Property.set(key, value, resource_id_or_key)
-        rest_status_ok
-      rescue Exception => ex
-        rest_status_ko(ex.message, 400)
-      end
-    else
-      rest_status_ko('Property key [' + params[:id] + '] is not valid', 400)
-    end
+    update
   end
 
+  # curl -u admin:admin -v -X POST http://localhost:9000/api/properties/foo?value=bar[&resource=<resource>]
   def update
     key = params[:id]
+    bad_request('missing key') unless key.present?
     value = params[:value] || request.raw_post
     resource_id_or_key = params[:resource]
     if resource_id_or_key
       resource = Project.by_key(resource_id_or_key)
-      if resource
-        resource_id_or_key = resource.id
-      else
-        rest_status_ko('Resource [' + resource_id_or_key + '] does not exist', 404)
-        return
-      end
+      not_found('resource not found') unless resource
+      resource_id_or_key = resource.id
     end
-    if key
-      begin
-        Property.set(key, value, resource_id_or_key)
-        rest_status_ok
-      rescue Exception => ex
-        rest_status_ko(ex.message, 400)
-      end
+    prop=Property.set(key, value, resource_id_or_key)
+    if prop.valid?
+      render_success('property created')
     else
-      rest_status_ko('Property key [' + params[:id] + '] is not valid', 400)
+      render_bad_request(prop.validation_error_message)
     end
   end
 
+  # curl -u admin:admin -v -X DELETE http://localhost:9000/api/properties/foo[?resource=<resource>]
   def destroy
     key = params[:id]
+    bad_request('missing key') unless key.present?
     resource_id_or_key = params[:resource]
     if resource_id_or_key
       resource = Project.by_key(resource_id_or_key)
       if resource
         resource_id_or_key = resource.id
       else
-        rest_status_ko('Resource [' + resource_id_or_key + '] does not exist', 404)
-        return
-      end
-    end
-    if key
-      begin
-        Property.clear(key, resource_id_or_key)
-        rest_status_ok
-      rescue Exception => ex
-        rest_status_ko(ex.message, 400)
+        # TODO should we ignore this error ?
+        not_found('resource not found')
       end
-    else
-      rest_status_ko('Property key [' + params[:id] + '] is not valid', 400)
     end
+    Property.clear(key, resource_id_or_key)
+    render_success('property deleted')
   end
 
-  protected
+  private
 
-  def rest_to_json(properties)
-    JSON(properties.collect{|property| property.to_hash_json})
+  def to_json(properties)
+    properties.collect { |property| property.to_hash_json }
   end
 
-  def rest_to_xml(properties)
+  def to_xml(properties)
     xml = Builder::XmlMarkup.new(:indent => 0)
     xml.instruct!
     xml.properties do
@@ -146,8 +112,6 @@ class Api::PropertiesController < Api::RestController
     end
   end
 
-  private
-
   def viewable?(property_key)
     !property_key.to_s.index('.secured') || is_admin?
   end
diff --git a/sonar-server/src/main/webapp/WEB-INF/app/models/property.rb b/sonar-server/src/main/webapp/WEB-INF/app/models/property.rb
index b388a5bec6b..4e407a906fe 100644
--- a/sonar-server/src/main/webapp/WEB-INF/app/models/property.rb
+++ b/sonar-server/src/main/webapp/WEB-INF/app/models/property.rb
@@ -50,9 +50,16 @@ class Property < ActiveRecord::Base
   end
 
   def self.set(key, value, resource_id=nil)
-    Property.delete_all('prop_key' => key, 'resource_id' => resource_id, 'user_id' => nil)
-    Property.create(:prop_key => key, :text_value => value.to_s, :resource_id => resource_id)
-    reload_java_configuration
+    prop = Property.new(:prop_key => key, :text_value => value.to_s, :resource_id => resource_id)
+    if prop.valid?
+      Property.transaction do
+        Property.delete_all('prop_key' => key, 'resource_id' => resource_id, 'user_id' => nil)
+        if prop.save
+          reload_java_configuration
+        end
+      end
+    end
+    prop
   end
 
   def self.clear(key, resource_id=nil)
@@ -95,6 +102,14 @@ class Property < ActiveRecord::Base
       end
   end
 
+  def validation_error_message
+    msg=''
+    errors.each_full do |error|
+      msg += Api::Utils.message("property.error.#{error}")
+    end
+    msg
+  end
+
   private
 
   def validate
diff --git a/sonar-server/src/main/webapp/WEB-INF/app/views/settings/_properties.html.erb b/sonar-server/src/main/webapp/WEB-INF/app/views/settings/_properties.html.erb
index fc2fbd2ae09..2508e29c272 100644
--- a/sonar-server/src/main/webapp/WEB-INF/app/views/settings/_properties.html.erb
+++ b/sonar-server/src/main/webapp/WEB-INF/app/views/settings/_properties.html.erb
@@ -53,19 +53,17 @@
                 <% end %>
                 <div><%= render :partial => "settings/type_#{property_type}", :locals => {:property => property, :value => value} -%></div>
                 <%
-                   if p
-                     p.errors.each_full do |error|
+                   if p && !p.valid?
                 %>
-                    <div class="error"><%= message("property.error.#{error}") -%></div>
+                    <div class="error"><%= p.validation_error_message -%></div>
                   <%
                      end
-                     end
                   %>
                 <p>
                   <%
                      default_prop_value = (@project ? Property.value(property.key(), nil, property.defaultValue()) : property.defaultValue())
                      unless default_prop_value.blank? %>
-                    <span class="note">Default : <%= h default_prop_value -%></span>
+                    <span class="note">Default : <%= property.getType().to_s=='PASSWORD' ? '********' : h(default_prop_value) -%></span>
                   <% end %>
                 </p>
               </td>