Wink
  1. Wink
  2. WINK-10

unnecessary constructor validation on subresource

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Duplicate
    • Affects Version/s: None
    • Fix Version/s: 0.1
    • Component/s: Common, Server
    • Labels:
      None

      Description

      Because a subresource does not have to conform to the JAX-RS rules regarding constructors, the code should not be enforcing these rules on subresources.

      Attached is a testcase to demo the problem. The hack I used to confirm that the runtime is enforcing the constructor rules on the subresource was to comment out the "continue L1;" call in AbstractMetadataCollector.parseConstructors

      The fix will be to make the AbstractMetadataCollector aware of what type of resource it is operating on; top-level resource vs. subresource.

      I'll work on this.

      1. updated-patch-gallardo.txt
        1 kB
        Nick Gallardo
      2. patch_fixcandidate_01.txt
        22 kB
        Mike Rheinheimer
      3. patch.txt
        8 kB
        Mike Rheinheimer

        Issue Links

          Activity

          Hide
          Mike Rheinheimer added a comment -

          Fix candidate patch attached. Basically, I added a method param to pass a boolean 'isSubResource' down into the AbstractMetadataCollector constructor so the AbstractMetadataCollector.parseConstructors() method is aware that it might be looking at a subresource.

          see patch_fixcandidate_01.txt

          Show
          Mike Rheinheimer added a comment - Fix candidate patch attached. Basically, I added a method param to pass a boolean 'isSubResource' down into the AbstractMetadataCollector constructor so the AbstractMetadataCollector.parseConstructors() method is aware that it might be looking at a subresource. see patch_fixcandidate_01.txt
          Hide
          Mike Rheinheimer added a comment -

          Also, I'm not tied to the fix suggested by fixcandidate_01 patch. Please review and critique or change if needed – or send comments and ideas my way.

          Also, FYI, I had a two test failures and two test errors in common, but they were unrelated to this patch. The failures were due to the position of the namespace declarations in some XML being checked in an assert. I did not check on the errors, but it looked like NPEs coming from a missing application validator object?? Anyway, this fix patch didn't cause those.

          Show
          Mike Rheinheimer added a comment - Also, I'm not tied to the fix suggested by fixcandidate_01 patch. Please review and critique or change if needed – or send comments and ideas my way. Also, FYI, I had a two test failures and two test errors in common, but they were unrelated to this patch. The failures were due to the position of the namespace declarations in some XML being checked in an assert. I did not check on the errors, but it looked like NPEs coming from a missing application validator object?? Anyway, this fix patch didn't cause those.
          Hide
          Nick Gallardo added a comment -

          Thanks for the patch Mike. I'll take a look.

          Show
          Nick Gallardo added a comment - Thanks for the patch Mike. I'll take a look.
          Hide
          Nick Gallardo added a comment -

          Mike, do you mind if I merge this test with another test I had created for polymorphic sub-resources?

          Show
          Nick Gallardo added a comment - Mike, do you mind if I merge this test with another test I had created for polymorphic sub-resources?
          Hide
          Mike Rheinheimer added a comment -

          That's fine. I think it would make sense to have subresource tests together.

          Show
          Mike Rheinheimer added a comment - That's fine. I think it would make sense to have subresource tests together.
          Hide
          Nick Gallardo added a comment -

          Mike, here's a different solution. Less impactful in that it doesn't require every caller to acknowledge whether or not it's a sub resource.

          Show
          Nick Gallardo added a comment - Mike, here's a different solution. Less impactful in that it doesn't require every caller to acknowledge whether or not it's a sub resource.
          Hide
          Mike Rheinheimer added a comment -

          Hi Nick. That looks fine to me. Will you commit it?

          Show
          Mike Rheinheimer added a comment - Hi Nick. That looks fine to me. Will you commit it?
          Hide
          Nadav Fischer added a comment -

          Nick, you can resolve this issue without applying the patch because the fix for WINK-83 also fixes this issue.

          Show
          Nadav Fischer added a comment - Nick, you can resolve this issue without applying the patch because the fix for WINK-83 also fixes this issue.
          Hide
          Mike Rheinheimer added a comment -

          Thanks Nadav. I've verified this is resolved by WINK-83. Thanks too for adding tests!

          Show
          Mike Rheinheimer added a comment - Thanks Nadav. I've verified this is resolved by WINK-83 . Thanks too for adding tests!

            People

            • Assignee:
              Nick Gallardo
              Reporter:
              Mike Rheinheimer
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development