search

grnet / djnro

DjNRO hits the decks of eduroam database management
http://djnro.grnet.gr/
Other
10 stars 21 forks source link

CATv2 API changes #57

Closed ghalse closed 5 years ago

ghalse commented 5 years ago

With the release of CAT 2.0-beta1 comes a significant change in the CAT admin API. This change is not backwards compatible, and the old API version is no longer supported in production.

DjNRO abstracts its CAT API calls into a CatQuery class, but the parameters passed to functions in this class are explicitly derived from v1 of the CAT API. This change introduces support for the CATv2 admin API while preserving the DjNRO CatQuery abstraction interface. By doing this, we avoid making changes to other parts of the DjNRO code base at the expense of slightly complicating the CatQuery class.

Part of the reason I've elected to do things in this way is that the CatQuery class contains functions and static methods that are not called from elsewhere within the published DjNRO code base, making it unclear as to why they exist -- and leading to the assumption that they're used by other, unpublished utilities.

zmousm commented 5 years ago

Sorry for the very very long delay.

To quote the author: the CAT version 1 admin API was "a terrible ad-hoc hack". You would send it form data and it would respond with XML. The input was somewhat peculiar (quoting public documentation, again), as one was expected to embed indexing and data type info in the name of the form-field. The output was some times invalid XML (e.g. for some errors). DjNRO tried to cope with that, but it had limited scope so it made no sense to fully abstract all aspects of such a "draft" API in CatQuery.

Part of the reason I've elected to do things in this way is that the CatQuery class contains functions and static methods that are not called from elsewhere within the published DjNRO code base, making it unclear as to why they exist -- and leading to the assumption that they're used by other, unpublished utilities.

I didn't understand this statement; what were the functions and static methods you referred to?

Anyway, thank you very much for picking this up. I pushed a couple of tweaks/fixes and rebased as necessary after the Django 1.11 update. I would have preferred to also maintain compatibility with the old CAT API. In order to achieve that, I would reverse the deuglify processing you introduced, so that input to CatQuery methods aligns with the new API, but it may also be translated to parameters for the old (and more limited) API. I want to revisit this, but it is not a topmost priority.