Uploaded image for project: 'OFBiz'
  1. OFBiz
  2. OFBIZ-8722

Update Services set non-pk fields to null, if only passed pks

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Not A Problem
    • Affects Version/s: Trunk
    • Fix Version/s: None
    • Component/s: ALL COMPONENTS
    • Labels:
      None

      Description

      When we pass only pks in some update services. All non-pk values are getting null.

        Issue Links

        There are no Sub-Tasks for this issue.

          Activity

          Hide
          deepak.dixit Deepak Dixit added a comment -

          With this fix we fixed an issue that was setting value null if not passed, but what if we want to set an field empty.

          We need to figure out a way to set value empty like set-if-empty attribute of set field.

          Show
          deepak.dixit Deepak Dixit added a comment - With this fix we fixed an issue that was setting value null if not passed, but what if we want to set an field empty. We need to figure out a way to set value empty like set-if-empty attribute of set field.
          Hide
          rishisolankii Rishi Solanki added a comment -

          I would say, the fix designed for the problem reported here is not appropriate and it is tightly coupled for certain scenario, which can not be handled by the user of the service. A service should be enough flexible which will give option user on how to use it. Before applying the fix as a user I will have option to get the records on PKs and change the non PK as per my requirement. After this change user always requires to put some values in the service parameters.

          IMO it is violating the loose coupling. We should rethink on weather the reported problem was really a problem? and if so then on solution provided for this?

          Show
          rishisolankii Rishi Solanki added a comment - I would say, the fix designed for the problem reported here is not appropriate and it is tightly coupled for certain scenario, which can not be handled by the user of the service. A service should be enough flexible which will give option user on how to use it. Before applying the fix as a user I will have option to get the records on PKs and change the non PK as per my requirement. After this change user always requires to put some values in the service parameters. IMO it is violating the loose coupling. We should rethink on weather the reported problem was really a problem? and if so then on solution provided for this?
          Hide
          deepak.dixit Deepak Dixit added a comment -

          I am saying the same Rishi, As per existing implementation if you only pass the limited non-pk filed than service automatically set all remaining non-pk fields to null this. To fix this issue here set-if-null it set to false. But it introduced new difficulty or issue to mange the empty value case. and I propose to fix this as well so that as a user we are free to manage it at service calling level.

          >>Before applying the fix as a user I will have option to get the records on PKs and change the non PK as per my requirement.

          I think here you are referring custom services, or wrapper services. But if we call CRUD services directly from controller than this was not working as expected and it set all the values to null.

          Show
          deepak.dixit Deepak Dixit added a comment - I am saying the same Rishi, As per existing implementation if you only pass the limited non-pk filed than service automatically set all remaining non-pk fields to null this. To fix this issue here set-if-null it set to false. But it introduced new difficulty or issue to mange the empty value case. and I propose to fix this as well so that as a user we are free to manage it at service calling level. >>Before applying the fix as a user I will have option to get the records on PKs and change the non PK as per my requirement. I think here you are referring custom services, or wrapper services. But if we call CRUD services directly from controller than this was not working as expected and it set all the values to null.
          Hide
          rishisolankii Rishi Solanki added a comment - - edited

          Yeah, calling it from controller requires an UI, and if user wants to send few values null then she can not. Same applies everywhere service will be used. That is why I'm not agree with the solution. Even with the problem reported, there is nothing wrong if service set any value what she gets from parameters, it is service user class/method responsiblity to take care of different behavior of basic crud service.

          Show
          rishisolankii Rishi Solanki added a comment - - edited Yeah, calling it from controller requires an UI, and if user wants to send few values null then she can not. Same applies everywhere service will be used. That is why I'm not agree with the solution. Even with the problem reported, there is nothing wrong if service set any value what she gets from parameters, it is service user class/method responsiblity to take care of different behavior of basic crud service.
          Hide
          deepak.dixit Deepak Dixit added a comment -

          It will fail in case of Ajax request calling like in in-place editing.

          If case of in-line/in-place editing we pass only primary key and related values. In this case it will set all the remaining items to null and its not an valid case.

          Like if you want to only edit PartyGroup.groupName and service set all the remaining parameters to null than its wrong workflow. And now a days SPA is mostly used application and user generally prefer to use agular/ajax for client side implementation so setting null values to remaining fields is not an good solution.

          Show
          deepak.dixit Deepak Dixit added a comment - It will fail in case of Ajax request calling like in in-place editing. If case of in-line/in-place editing we pass only primary key and related values. In this case it will set all the remaining items to null and its not an valid case. Like if you want to only edit PartyGroup.groupName and service set all the remaining parameters to null than its wrong workflow. And now a days SPA is mostly used application and user generally prefer to use agular/ajax for client side implementation so setting null values to remaining fields is not an good solution.
          Hide
          rishisolankii Rishi Solanki added a comment -

          Ajax request or in place editing or UI or wrapper service or any other place. For all I'm saying, if user do wants to set null for non-pk then she can do that with previous version of service. With this updated version of service implemented in this ticket from all these places user can not set the null value.

          Many times service user or end user requires to set null values for a field intentionally. And if service is not allowing user to do that then she has only one option to use delegator directly or may be write separate service to do that.

          Previouse version of service may have some problem which should be discussed and fixed. But the new version of service implemented here would increase the problem using itself (service). With previous version was flexible enough to take care of both scenario and with new version user has no option to handle the null case.

          Also angular js, in-place editing, client side implementation can easily add client side restriction. I'll check what goes wrong with Ajax/in-line/in-place/angular js if we use older version.

          My general understanding goes with older version of service for all the possible cases I could think of to update a record using service. If that version has some problem, then the solution provided here is not correct and increasing the complexity of the service IMO.

          See what others in the community has to say on this. Atleast I'm not in favor of this fix.

          Show
          rishisolankii Rishi Solanki added a comment - Ajax request or in place editing or UI or wrapper service or any other place. For all I'm saying, if user do wants to set null for non-pk then she can do that with previous version of service. With this updated version of service implemented in this ticket from all these places user can not set the null value. Many times service user or end user requires to set null values for a field intentionally. And if service is not allowing user to do that then she has only one option to use delegator directly or may be write separate service to do that. Previouse version of service may have some problem which should be discussed and fixed. But the new version of service implemented here would increase the problem using itself (service). With previous version was flexible enough to take care of both scenario and with new version user has no option to handle the null case. Also angular js, in-place editing, client side implementation can easily add client side restriction. I'll check what goes wrong with Ajax/in-line/in-place/angular js if we use older version. My general understanding goes with older version of service for all the possible cases I could think of to update a record using service. If that version has some problem, then the solution provided here is not correct and increasing the complexity of the service IMO. See what others in the community has to say on this. Atleast I'm not in favor of this fix.
          Hide
          deepak.dixit Deepak Dixit added a comment -

          It depends on use cases, in how many cases user really want to set value to null for all the fields??

          In my initial comment I was saying the same that this fixes an issue but need to discuss and finalize the solution for another one

          suggestions are most welcome.

          Show
          deepak.dixit Deepak Dixit added a comment - It depends on use cases, in how many cases user really want to set value to null for all the fields?? In my initial comment I was saying the same that this fixes an issue but need to discuss and finalize the solution for another one suggestions are most welcome.
          Hide
          deepak.dixit Deepak Dixit added a comment -

          Reverting this fixes for now at r#1769057,as right now I don't have much time to debug it in detail. Will debug it in more and commit it with proper fix else will discard this issue.

          Show
          deepak.dixit Deepak Dixit added a comment - Reverting this fixes for now at r#1769057,as right now I don't have much time to debug it in detail. Will debug it in more and commit it with proper fix else will discard this issue.
          Hide
          deepak.dixit Deepak Dixit added a comment -

          This is working expected, no need to set set-if-null true for set-nonpk-fields. As from runService screen we are passing all the values to empty/null so it set them to null, and this is expected behavior. I checked it if we call service from Ajax call and pass limited values than it working as expected.

          So closing this ticket as Not a problem

          Show
          deepak.dixit Deepak Dixit added a comment - This is working expected, no need to set set-if-null true for set-nonpk-fields. As from runService screen we are passing all the values to empty/null so it set them to null, and this is expected behavior. I checked it if we call service from Ajax call and pass limited values than it working as expected. So closing this ticket as Not a problem

            People

            • Assignee:
              deepak.dixit Deepak Dixit
              Reporter:
              pawan.verma Pawan Verma
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development