Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CSW Abstraction and XML Metadata Upload #248

Closed
wants to merge 103 commits into from
Closed

CSW Abstraction and XML Metadata Upload #248

wants to merge 103 commits into from

Conversation

tomkralidis
Copy link
Member

…_caps CSW param (to reduce bandwidth), prioritize catalogue.py by callers, optimize download url parsing
@dwins
Copy link
Contributor

dwins commented May 30, 2012

does this branch still cover GNIP's 4 and 5?

@jj0hns0n
Copy link
Member

At this point it does. I believe Tom is working on getting the tests
to pass after a big refactor we did together on friday and then will
work on splitting out the xml upload functionality into another
branch.

On Wed, May 30, 2012 at 9:06 AM, David Winslow
reply@reply.github.com
wrote:

does this branch still cover GNIP's 4 and 5?


Reply to this email directly or view it on GitHub:
#248 (comment)

@tomkralidis
Copy link
Member Author

@dwins yes. I will pull out the GNIP 4 related functionality from this pull request.

@@ -1249,122 +1274,42 @@ def search_result_detail(request):
layer = None
layer_is_remote = True

keywords = []
if hasattr(rec, 'identification') and hasattr(rec.identification, 'keywords'):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this belong in the catalog backend? If our catalog abstraction layer doesn't even provide a consistent interface (requiring hasattr) I would argue that it's too low-level. Let's shoot for this method to look more like:

def search_result_detail(request):
    uuid = request.GET.get("uuid")
    catalogue = get_catalogue()
    record = catalogue.get_by_uuid(uuid)
    return render_to_response('maps/search_result_snippet.html',
       RequestContext(request, { 'record' : record }))

The idea here is that all the matching up of Layer to metadata and massaging into a template-ready format is done by the catalog backend.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OWSLib already provides an abstracted request/response model; don't we already have an abstraction of CSW in OWSLib itself? If catalogue/catalogue.py commits to an OWSLib interface, with ISO as the metadata model, isn't this enough? I think it may be too much bloat too abstract from OWSLib a level higher, not to mention transforming many objects at runtime when we already have them in OWSLib's CSW and ISO abstraction.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't say anything about transforming objects, just putting the logic of matching Layers and Records into the backend instead of having it in the views. I would argue that this is more about choosing appropriate abstractions than adding new ones - is a catalog backend just a slightly smarter wrapper around owslib's CatalogueServiceWeb, or is it responsible for bridging the divide between Django models and Catalog records? I think we can choose the latter without adding bloat here, and isolating the catalogue record-wrangling code from the template-slinging seems worth it to me.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I second David's opinion here.

On Tue, Jun 5, 2012 at 11:31 AM, David Winslow
reply@reply.github.com
wrote:

@@ -1249,122 +1274,42 @@ def search_result_detail(request):
         layer = None
         layer_is_remote = True

  •    keywords = []
  •    if hasattr(rec, 'identification') and hasattr(rec.identification, 'keywords'):

I didn't say anything about transforming objects, just putting the logic of matching Layers and Records into the backend instead of having it in the views.  I would argue that this is more about choosing appropriate abstractions than adding new ones - is a catalog backend just a slightly smarter wrapper around owslib's CatalogueServiceWeb, or is it responsible for bridging the divide between Django models and Catalog records?  I think we can choose the latter without adding bloat here, and isolating the catalogue record-wrangling code from the template-slinging seems worth it to me.


Reply to this email directly or view it on GitHub:
https://github.com/GeoNode/geonode/pull/248/files#r929575

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this approach more acceptable to put forth, then? : https://gist.github.com/2876384

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah Tom, That looks like it! Thanks.


connection = settings.CSW['default']

class Catalogue(CatalogueServiceWeb):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Catalogue abstraction in GeoNode should probably use encapsulation rather than inheritance to access OWSLib functionality - ie, maintain an owslib.csw.CatalogueServiceWeb instance as a member variable, rather than inheriting from it. That makes it explicit where we are using catalogue.Catalogue methods instead of ones from OWSLib (and we can easily review the code to minimize such access.)

@tomkralidis
Copy link
Member Author

FYI the GNIP 4 (XML metadata upload) specific functionality has been removed from this pull request (tomkralidis@00d3e17) and will be recast in a subsequent pull request.

else:
return self.keywords_region

def thumbnail(self):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this thumbnail used as part of the refactor?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jj0hns0n jj0hns0n closed this Jun 22, 2012
afabiani pushed a commit that referenced this pull request Jun 5, 2018
travislbrundage pushed a commit to travislbrundage/geonode that referenced this pull request Sep 5, 2018
marthamareal pushed a commit to marthamareal/geonode that referenced this pull request Sep 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants