Invalid LidvidsContext bounds fails internally and returns http500, vs expected handle and http400

[alexdunnjpl]

💡 Description

ex.
http://localhost:8080/collections/urn:nasa:pds:mars2020_supercam:data_calibrated_audio::4.0/products?start=0&limit=-1
http://localhost:8080/collections/urn:nasa:pds:mars2020_supercam:data_calibrated_audio::4.0/products?start=-1&limit=1
http://localhost:8080/collections/urn:nasa:pds:mars2020_supercam:data_calibrated_audio::4.0/products?start=-1&limit=-1

All three should throw during URIParameters instantiation, with exceptions being caught and handled.

[alexdunnjpl]

@al-niessner @nutjob4life @tloubrieu-jpl @jimmie are the javax API parameter validation annotations purely for documentation purposes, or are they expected to trigger enforcement behaviour?

I'm not sure whether the validation isn't working, or whether I need to roll my own handler for IllegalArgumentExceptions somewhere up the chain.

[alexdunnjpl]

On hold pending additional info

[al-niessner]

@alexdunnjpl @tloubrieu-jpl

It has been my experience that they URL parameter limits are enforced - limit as a letter did not work. Also limit used to be 1 to infinity and I changed it to 0 to infinity to get it working.

That said, I know the code generator is being changed so I do not know for certain anymore.

[alexdunnjpl]

Thanks Al.

It doesn't appear that they're being respected/enforced, so that's something I'll need to follow up on I think.

Unrelated relevant note:

• 3. The default values for limit and start work in conjunction later in the
• business logic to indicate a Singular or Plural return type. See swagger.yml
• to understand Singular and Plural return types.

This indicates that stripping out isSingular() as a side-effect of excising isSummaryOnly() et al. per @tloubrieu-jpl 's input (Slack) may break something, and it may be necessary to understand and reimplement that functionality.

It looks as though the unintuitive URIParameters.start default value of -1 is used as some kind of flag to indicate whether the API endpoint is a Singular or Plural (i.e. collection?) return.

[alexdunnjpl]

The only use of isSingular() is in RequestAndResponseContext.setResponse().

Seems like Singular/Plural-ness is an inherent property of an endpoint and should therefore be set in every SwaggerJava*Transmuter method (i.e. every route), manually. This should not be done in the chained call, I think, as it must happen before setResponse() which is problematically-brittle but is probably a lot of work to fix properly.

[al-niessner]

Thanks Al.

It doesn't appear that they're being respected/enforced, so that's something I'll need to follow up on I think.

Unrelated relevant note:

• 3. The default values for limit and start work in conjunction later in the
• business logic to indicate a Singular or Plural return type. See swagger.yml
• to understand Singular and Plural return types.

This indicates that stripping out isSingular() as a side-effect of excising isSummaryOnly() et al. per @tloubrieu-jpl 's input (Slack) may break something, and it may be necessary to understand and reimplement that functionality.

It looks as though the unintuitive URIParameters.start default value of -1 is used as some kind of flag to indicate whether the API endpoint is a Singular or Plural (i.e. collection?) return.

@alexdunnjpl

Yes, I do a lot of not-intuitive things. Using an illegal value (not accepted as input) allows one to understand if it has been set or not. Therefore, in the case of Plural where the starting number is required and defaults to 0 or 1, then the -1 gets overridden. When it is Singular, it is not part of the URL so it remains -1, when the user should not be able to define. So it seems an obvious and direct measure of Plural or Singular even if not so intuitive.

Also, sorry the chaining is causing you problems too. I find chaining easier to read than 400 lines of longvariablename.setsomething(); because the longvariablename is just repeated for no real value.

[alexdunnjpl]

@al-niessner in case it needs saying, my phrasing is an attempt to document my thought process, and never rarely an attempt to make a value judgement.

No problem with the chaining, just jotting down that if that's the route I end up going down I need to remember not to tuck the "setSingularness()" in the method chain because method chains which break quietly when the order isn't what it needs to be is a great way to end up with a bunch of wasted time down the line.

Regarding the "has this value been set"? check, is there any reason that springs to mind that would preclude

• using null
• using an additional explicit variable (in this case, probably something like isSingular which defaults to False and is explicitly set to true for those endpoints where the return is a single datum, like /products/{someIdentifier}) - this would be my preference
• using an explicit variable, which defaults to True and is set to false iff setStart() or setLimit() is called

[alexdunnjpl]

At first glance it seems like the approaches which resolve singular-ness from mutation of start or limit values may break anyway in the case that the user erroneously provides limit or start query params to an endpoint which doesn't support them, as I haven't yet found anything that would prevent these erroneous values from being assigned to the URIParameters/UserContext in such a case.

Testing shows it does not break - this is possibly prevented by setLimit() and setStart() not being called in singular-type endpoint route method(?) Update: Yes

[al-niessner]

@alexdunnjpl

Chaining order does not matter. You should not need a isSIngular() item or if you do just use start being an illegal value in the user parameters. Chaining is a pain in that it has to return the object rather than void when doing a set. Looks odd but most people recognize it as chaining.

Never use null (also) because it is not an object it is a specific pointer. If you never use null but rather null objects like RequestNull that has behavior for all the methods that do what you want it to do when it should be null, then it saves you billions of lines of code that are if (x == null) that just pollutes code. Like Nancy said in the 80s, just say NO.

You can find a billion more of those articles if you want. Same also goes for None in python although python does do a way better job of smoothing it over.

[al-niessner]

Singular means 1 and so the URL does not allow those parameters. If it does, then no longer Singular.

…

On Tue, Feb 21, 2023, 14:03 Alex Dunn ***@***.***> wrote: At first glance it seems like the approaches which resolve singular-ness from mutation of start or limit values may break anyway in the case that the user erroneously provides limit or start query params to an endpoint which doesn't support them, as I haven't yet found anything that would prevent these erroneous values from being assigned to the URIParameters/ UserContext in such a case. — Reply to this email directly, view it on GitHub <#252 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAIUBIT37CVJJDNHYKKJCZDWYU3T3ANCNFSM6AAAAAAVDOQI3I> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[alexdunnjpl]

Based on what I'm seeing, it seems like the best way to explicitly capture what's going on is to add the default-true isSingular member and mutate it to false during setLimit and setStart`, as that's most-consistent with the existing behaviour (i.e. routes being non-singular by virtue of making those calls in the route methods).

@al-niessner is there any existing functionality for returning a valid, non-HTTP200 response on particular exceptions which have bubbled up from the internals of the API route behaviour? I couldn't find any (but that doesn't mean a whole lot).

[al-niessner]

@alexdunnjpl

Mutating the isSingular during set{Start,Limit} is the correct solution.

The longer answer is validating values. The short version is move a comment block from a generated interface to SwaggerJavaTransmutter override and test to see if the parameters are checked by somebody else like springsomething. Knowing that @controller does not walk back the ancestry means it probably cannot find those comments that bound the parameters. Moving the comment block over is the correct solution if it works - I quit doing it because I was changing the yaml frequently and extensively.

If copying the comment blocks do not work, then mutating isSingular will allow a validate to take place and then should be down in the controller package in the SwaggerJavaTransmutter.process() that has the big try/catch block that transmutes exceptions to http errors. It should also check all parameters before generating the exception - its how the PDS folks like validate to work too.

If the short version does not make sense, let me know and I will post a horribly long version with links to the code but it will take me a while.

[alexdunnjpl]

@jordanpadams @tloubrieu-jpl what's the priority on this? (I'm thinking not high enough to justify untangling the knot @al-niessner refers to above, at least while we each have other urgent items).

[jordanpadams]

@alexdunnjpl I think this can be added to the list, but not super critical

[al-niessner]

@alexdunnjpl @tloubrieu-jpl

I was being a bit overzealous when marking related tickets. This is an eclipse problem with @controller annotation. I think it may be related to api docs too. I used the eclipse tool to create the override methods and I do not think it brought enough with it. My hypothesis needs to be tested, but if we copy the comments and complete method definition from the auto-generated interface it should do the parameter validation as well as put the methods in their correct grouping in the docs. It is probably wiser to wait until the number of endpoints is scaled back to then do the quintessential antipattern (cut-n-paste) to fix these problems - the irony is awesome.

[tloubrieu-jpl]

I would be ok to apply that ticket only for the end point we want to maintain and ignore the deprecated ones, if that helps.

[jordanpadams]

Deferring this bug to B14.0