dadi / publish

Publish provides beautiful editorial interfaces for the management of content within API
https://dadi.cloud/en/publish
64 stars 13 forks source link

Message "API failed to respond" when API collection schema is invalid #362

Open jimlambie opened 6 years ago

jimlambie commented 6 years ago

Publish fails here https://github.com/dadi/publish/blob/master/app/lib/helpers/collection-routes.js#L145 when an API collection schema Reference field doesn't include a "settings" object.

API should handle the validation for this, however Publish might also check for this property before accessing it.

eduardoboucas commented 6 years ago

Worth noting that since API 3.1, it's not mandatory that Reference fields include a settings.collection property, as they can reference multiple collections. More info here: https://docs.dadi.cloud/api/3.2#multi-collection-references

danwdart commented 6 years ago

Does that mean the requirements for this ticket have changed? Should there be separate tickets for Publish and API? So is it Publish to just check / ignore if fail here and then another ticket for API to deal with validation on a bigger bit?

danwdart commented 6 years ago

@jimlambie what's the updated location?

jimlambie commented 6 years ago

@danwdart an excellent question, and not one I can answer... @eduardoboucas ?

danwdart commented 6 years ago

Are we saying that we should get an "API failed to respond" or we do and we shouldn't? I have tried this and all I got was the field being missing from Publish's view.

danwdart commented 6 years ago

the answer to this was that this message shouldn't appear. Turns out what I've tried is actually changing settings.collection to a nonsense settings.collections with an array to see if it'd complain. So settings.collection isn't mandatory - and looks like from the docs that settings isn't either - but wouldn't it make sense to restrict a list of known / allowed collections rather than allowing just any collection to be referenced in the instructions for multiple? What do you think @eduardoboucas @jimlambie ?

eduardoboucas commented 6 years ago

So settings.collection isn't mandatory - and looks like from the docs that settings isn't either

Correct 👍

wouldn't it make sense to restrict a list of known / allowed collections rather than allowing just any collection to be referenced in the instructions for multiple?

I think this is a question for API, not Publish. We're revisiting validation as part of https://github.com/dadi/api/issues/451 and I'd say this is part of that.

danwdart commented 6 years ago

I'm back onto this and last I saw was that I had a little trouble figuring out that if there was no settings.collection how I should deal with it later, since not having one had a problem later in the function. But will investigate nonetheless.

danwdart commented 6 years ago

I think this has been fixed by other code in passing. I'm not getting any such error anymore without the settings block - I'm just not seeing the field appear.

danwdart commented 6 years ago

This should make the field appear regardless - so this is in another part of the code

danwdart commented 6 years ago

Parking as per @jimlambie 's recommendation