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

feature (webapi): make project shortcode required #824

Merged
merged 32 commits into from
Apr 26, 2018

Conversation

subotic
Copy link
Collaborator

@subotic subotic commented Apr 12, 2018

Summary

  • make project shortcode required
  • add shortcode to projects that did not have one up until now (incunabula, biblio and anything)
  • changed dokubib shortcode to one from the production range as these projects are going to be added in production
  • had to revert some changes in 7687651 and 90a68aa to make it work again. These changes are backed by two new tests. Maybe you could take a look to see if I understood it correctly and that the tests are correct.
  • change all data.knora.org to rdfh.ch

Issues

@subotic subotic added the enhancement improve existing code or new feature label Apr 12, 2018
@subotic subotic added this to the v1.4.0 milestone Apr 12, 2018
@subotic subotic self-assigned this Apr 12, 2018
@subotic
Copy link
Collaborator Author

subotic commented Apr 13, 2018

@benjamingeer so this is more work then I anticipated.

I'm not sure if I have broken something or if simply the expected result needs to be changed.

Test output of JSONLDHandlingV2R2RSpec.scala:

Map("rdf" -> "http://www.w3.org/1999/02/22-rdf-syntax-ns#", "knora-api" -> "http://api.knora.org/ontology/knora-api/v2#", "schema" -> "http://schema.org/", "rdfs" -> "http://www.w3.org/2000/01/rdf-schema#", "p0803-incunabula" -> "http://0.0.0.0:3333/ontology/0803/incunabula/v2#") did not equal Map("rdf" -> "http://www.w3.org/1999/02/22-rdf-syntax-ns#", "knora-api" -> "http://api.knora.org/ontology/knora-api/v2#", "schema" -> "http://schema.org/", "rdfs" -> "http://www.w3.org/2000/01/rdf-schema#", "incunabula" -> "http://0.0.0.0:3333/ontology/0803/incunabula/v2#") @context incorrect (JSONLDHandlingV2R2RSpec.scala:113)

In particular, "p0803-incunabula" -> "http://0.0.0.0:3333/ontology/0803/incunabula/v2#" vs "incunabula" -> "http://0.0.0.0:3333/ontology/0803/incunabula/v2#". Is p0803-incunabula the correct result?

@benjamingeer
Copy link

p0803-incunabula is correct. StringFormatter makes prefix labels like this because there might be two different ontologies called incunabula in different projects.

@benjamingeer
Copy link

Does this branch include the commits I did to require project shortcodes in #788?

bb930c4

3698588

@benjamingeer
Copy link

Never mind, I just saw that it does include them. :)

@subotic
Copy link
Collaborator Author

subotic commented Apr 13, 2018

thanks :-)

# Conflicts:
#	webapi/src/test/scala/org/knora/webapi/responders/v2/OntologyResponderV2Spec.scala
@subotic subotic added breaking any breaking change breaking (existing data) and removed breaking any breaking change labels Apr 23, 2018
@subotic
Copy link
Collaborator Author

subotic commented Apr 24, 2018

@benjamingeer whenever you have some spare time :-)

} else {
s"http://$IriDomain/${projectInfo.shortname}/$knoraResourceID"
}
s"http://$IriDomain/${projectInfo.shortcode}/${projectInfo.shortname}/$knoraResourceID"

Choose a reason for hiding this comment

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

Why include both the project shortcode and the project shortname?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch. Only the shortcode would be more than enough.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done


<http://data.knora.org/0C-0L1kORryKzJAJxxRyRQ> a anything:Thing ;
<http://rdfh.ch/0C-0L1kORryKzJAJxxRyRQ> a anything:Thing ;

Choose a reason for hiding this comment

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

If you really want to make this standard, it would have to include the project shortcode as in KnoraIdUtil.makeRandomResourceIri.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, you are right.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

but this is another discussion. I will do it as described here

Copy link
Contributor

Choose a reason for hiding this comment

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

That is how we formatted our data too, so, so far, our prod data is compliant to the doc.

But our (Lausanne) next upgrade will be a big one, so I would be in favour to push as much changes as we want to push now and have smoother releases in the future.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

Choose a reason for hiding this comment

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

Did you want to do this for incunabula-data.ttl, too?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh, please no. This will take too long. The anything data is small, while incunabula is huge. I need to finish some things for Lukas before my vacation next week and after I'm back, prepare the production server, so I won't be working on Knora until June.

Choose a reason for hiding this comment

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

No problem, I was just wondering how masochistic you were feeling. :)

@@ -413,7 +416,7 @@ Die Internetpublikation macht das digitalisierte Korpus dieser Frühdrucke durc
# BEOL #
###################################

<http://rdfh.ch/projects/0801> a knora-base:knoraProject ;
<http://rdfh.ch/projects/yTerZGyxjZVqFMNNKXCDPF> a knora-base:knoraProject ;

Choose a reason for hiding this comment

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

Why does the BEOL project have a UUID in its IRI, while all the other projects have shortcodes in their IRIs?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because then I would also need to change the data on the production server, which I'm reluctant to do at the moment.

Choose a reason for hiding this comment

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

OK, no problem.

@@ -1779,7 +1781,7 @@ class StringFormatter private(val knoraApiHostAndPort: Option[String]) {
*/
def xmlImportNamespaceToInternalOntologyIriV1(namespace: String, errorFun: => Nothing): SmartIri = {
namespace match {
case ProjectSpecificXmlImportNamespaceRegex(_, Optional(projectCode), ontologyName) if !isBuiltInOntologyName(ontologyName) =>
case ProjectSpecificXmlImportNamespaceRegex(projectCode, ontologyName) if !isBuiltInOntologyName(ontologyName) =>

Choose a reason for hiding this comment

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

Sorry, this was my bug here, fixed now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

thanks :-)

@benjamingeer
Copy link

benjamingeer commented Apr 24, 2018

had to revert some changes in 7687651 and 90a68aa to make it work again

These were both my mistakes, sorry:

The code that uses the regex that matches project-specific API v1 XML import namespaces was incorrect (it expected the wrong number of capturing groups). I've fixed that, and made the project shortcode required in the regex again, and it works now.

The regex that matches API v2 ontology URL paths was incorrect because it can't require the shortcode (which I mistakenly made it do), because it handles built-in ontologies as well as project-specific ones, and built-in ontologies don't have shortcodes.

Thanks for the tests. :)

@subotic
Copy link
Collaborator Author

subotic commented Apr 24, 2018

Thanks for adding the fix :-)

@benjamingeer
Copy link

This was really a lot of work, thanks for being patient enough to do it.

@subotic
Copy link
Collaborator Author

subotic commented Apr 26, 2018

thanks :-)

@subotic subotic merged commit 66e03b3 into develop Apr 26, 2018
@subotic subotic deleted the wip/required-project-shortcode branch April 26, 2018 14:24
SepidehAlassi added a commit that referenced this pull request Apr 30, 2018
* develop:
  feature (webapi): make project shortcode required (#824)
  doc (webapi): jekyll / paradox based documentation website (#751)
  Validate ontologies loaded from the triplestore (#806)
  fix (webapi): add searchbox gui element to hasOtherSomething property (#828)
  feature (webapi): allow loading data into graphdb on windows (internal PR) (#814)
  build (webapi): add missing library (#822)
  feature (webapi): allow changing of user’s password by system admin (#815)
  feature (webapi): OpenAPI / Swagger API documentation generation (#812)
SepidehAlassi added a commit that referenced this pull request Apr 30, 2018
* develop:
  feature (webapi): make project shortcode required (#824)
  doc (webapi): jekyll / paradox based documentation website (#751)
  Validate ontologies loaded from the triplestore (#806)
  fix (webapi): add searchbox gui element to hasOtherSomething property (#828)
  feature (webapi): allow loading data into graphdb on windows (internal PR) (#814)
  build (webapi): add missing library (#822)
  feature (webapi): allow changing of user’s password by system admin (#815)
  feature (webapi): OpenAPI / Swagger API documentation generation (#812)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement improve existing code or new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Making Project-Shortcode required Convert existing ontology names to NCNames
3 participants