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

Update release schema to be coherent with merging specification #330

Open
kindly opened this issue May 18, 2016 · 21 comments · May be fixed by #1712
Open

Update release schema to be coherent with merging specification #330

kindly opened this issue May 18, 2016 · 21 comments · May be fixed by #1712
Assignees
Labels
Focus - Merging Relating to the creating of compiled releases and versioned releases Schema Relating to other changes in the JSON Schema (renamed fields, schema properties, etc.)
Milestone

Comments

@kindly
Copy link
Contributor

kindly commented May 18, 2016

As mentioned in #284 a compiledRelease in an OCDS record has the same schema as a standard OCDS release.
However, some of the fields, most notably the release ID and date, make no sense once compiled and they are also mandatory. This means it is not possible to make a compiled release that makes sense and validates.

The best approach would be to make a new sightly modified version of the OCDS release to take this into account. Naming possibly merged-release-scema.json

@kindly kindly self-assigned this May 18, 2016
@duncandewhurst duncandewhurst added this to the Version 1.1 milestone Nov 8, 2016
@duncandewhurst duncandewhurst added Focus - Documentation Includes corrections, clarifications, new guidance, and UI/UX issues Schema Relating to other changes in the JSON Schema (renamed fields, schema properties, etc.) labels Nov 8, 2016
@duncandewhurst
Copy link
Contributor

It's also worth noting that whilst the merging rules guidance states that the id and date fields should be omitted from a compiled release, the example data on the same page of the docs shows a compiled release with both of these fields included.

We should fix this as when we update the docs as part of the upgrade

@Bjwebb Bjwebb assigned Bjwebb and unassigned kindly Jan 31, 2017
@Bjwebb
Copy link
Contributor

Bjwebb commented Feb 14, 2017

@kindly Your current code drops tag from the versioned release. I'm not quite clear that this is the correct behaviour.

@Bjwebb
Copy link
Contributor

Bjwebb commented Feb 16, 2017

Just discussed with @kindly why dropping tag does make sense, because every versioned block has the associated releaseTag.

@jpmckinney
Copy link
Member

The issues that @kindly and @duncandewhurst reported haven't been fixed.

@jpmckinney jpmckinney reopened this Aug 26, 2017
@jpmckinney jpmckinney modified the milestones: Post 1.1 tasks, Version 1.1 Aug 26, 2017
@jpmckinney jpmckinney removed the Focus - Documentation Includes corrections, clarifications, new guidance, and UI/UX issues label Aug 26, 2017
@jpmckinney jpmckinney removed this from the Post 1.1 tasks milestone Sep 18, 2018
@jpmckinney jpmckinney added this to the 1.2 milestone Sep 18, 2018
@jpmckinney jpmckinney added the Focus - Merging Relating to the creating of compiled releases and versioned releases label Nov 27, 2018
@jpmckinney
Copy link
Member

jpmckinney commented Dec 6, 2018

Starting to document differences between release-schema.json and proposed compiled-release-schema.json:

  • id and date should be omitted from the schema (backwards-compatible)
  • tag should be ["compiled"], or it should be omitted from the schema, and 'compiled' should be removed from the releaseTag.csv codelist. I prefer the latter. Making both those changes at once would be backwards-compatible, as only compiled releases should be using the 'compiled' tag.
  • No fields should have null values (debatably backwards-incompatible)
  • See discussion on identifying (or at minimum dating) compiled releases in Clarify the scope of uniqueness of Release ID #455
  • If publisher is added to the release schemas, as in Add publisher field (release schema) #325, we might want its semantics to be different in the compiled release, i.e. to be the publisher of the compiled release, instead of following the regular merge rules (or, we might want to use a different field name for this).

@JachymHercher
Copy link
Contributor

tag should be ["compiled"], or it should be omitted from the schema, and 'compiled' should be removed from the releaseTag.csv codelist. I prefer the latter. Making both those changes at once would be backwards-compatible, as only compiled releases should be using the 'compiled' tag.

Basic question: why do compiled releases currently have releaseTag set only to 'compiled', instead of containing the tags of all the individual releases and, additionally, 'compiled'? Since the field is already there anyway, wouldn't the "adding" approach remove an anomaly and also have the added value of showing users at first sight how far along the contracting process has progressed?

@jpmckinney
Copy link
Member

jpmckinney commented Nov 16, 2021

Tags are not cumulative. The semantics of tag are "what is this release doing?" like "this release is amending the contract". If a prior release amended the tender, it would be incorrect to tag a later release about a contract update as ["tenderAmendment", "contractUpdate"].

That said, tag cannot have those same semantics in a compiled release (the compiled release is about many releases, not this release), and so we could introduce new semantics and accumulate the tags into an array (as you suggest) – instead of the options described previously (requiring that it be ["compiled"] or removing the field from the compiled release).

@ErinClark
Copy link

Hi @jpmckinney ,
I am also just trying to work out a solution for this for users who just want to get the compiled release, but also retain information on the history of which phases it is combining. I was going to add a top level field to the compiled release named 'compiled_tags' which would be an array of all previous tags. Does this sound reasonable?

@jpmckinney
Copy link
Member

@ErinClark Until this issue is resolved, it makes sense to add an additional field as you suggest.

@duncandewhurst
Copy link
Contributor

so we could introduce new semantics and accumulate the tags into an array (as you suggest) – instead of the options described previously (requiring that it be ["compiled"] or removing the field from the compiled release).

This would be useful for implementations that use the EU profile because it would be possible to check, using only the compiled release, if both a tender and award release had been published. For the UK, which has issues with linking tenders and awards, currently, you need to check individual releases.

@jpmckinney
Copy link
Member

Based on #1160 (comment), +1 to accumulating tags in compiled releases.

@jpmckinney jpmckinney self-assigned this Jun 7, 2023
@jpmckinney jpmckinney moved this to To do: Refactoring in OCDS 1.2 Jul 2, 2024
@odscjen
Copy link
Contributor

odscjen commented Jul 17, 2024

It's been a few years since the initial list of differences was made in #330 (comment), the following is the updated version based on the outcome of previously mentioned issues and further discussion in this and related threads.

  • id should be ocid and the maximum date among the individual releases used to create the compiled release, separated by a hyphen: {ocid}-{date}.
  • date should be the maximum date among the individual releases used to create the compiled release.
  • tag should include all tags from the individual releases and the additional ["compiled”] tag.
  • No fields should have null values (debatably backwards-incompatible)
  • publisher should be set to the publisher of the compiled release. This may be different from the publisher of the individual releases. But as the definition of the release publisher is “The original publisher of this release”, semantically the field definition doesn’t need to change.

@odscjen
Copy link
Contributor

odscjen commented Jul 17, 2024

@jpmckinney I've manually created a file compiled-release-schema.json using the list above and put it in a draft PR for now. I've assumed here that the purpose of having compiled-release-schema.json is to validate a compiled release against, not for tools creating the compiled release.

  • descriptions updated for id, tag, date and publisher and "omitWhenMerged" field removed
  • removed null as an option in arrays of strings to enforce the "No fields should have null values" element (which I'm aware causes a bunch of tests to fail, possibly adding minItems to all of those arrays is preferable as long as they're not required?).

Is this schema what you had in mind? If it is then I can move onto making it automatable by manage.py as part of the pre-commit step

@odscjen odscjen moved this from To do: Refactoring to In progress in OCDS 1.2 Jul 30, 2024
@odscjen
Copy link
Contributor

odscjen commented Oct 29, 2024

@duncandewhurst please see latest comments, do you think this is the way forward?

@duncandewhurst
Copy link
Contributor

Yep, I think the next step is to add this to the pre-commit command. Let me know if you want me to do that.

The compiled release schema looks good to me, except I don't think .tag should include 'compiled' - just the tags from the individual releases. Also, the schema's .title and .description will need to be updated. The description can reuse some of the language from https://standard.open-contracting.org/staging/1.2-dev/en/schema/records_reference/#compiled-release, https://standard.open-contracting.org/staging/1.2-dev/en/schema/merging/ and the description of .compiledRelease in the record schema.

In the release schema, we'll need to remove references to compiled releases from the descriptions of .id and .date. That can be done manually.

@odscjen
Copy link
Contributor

odscjen commented Oct 31, 2024

given that 'compiled' is an option in the releaseTag codelist I do think any compiled release should contain it in .tag, as well as all the tags from the individual releases. I had looked to see if there was a way of enforcing that in the schema but it doesn't look as though there is, not in this version of json schema at least, which is why I just added it to the tag description.

Good catch on updating the schema title and description, I'll do that in the draft PR.

If you could add this to the pre-commit command that would be great!

@jpmckinney
Copy link
Member

jpmckinney commented Oct 31, 2024

Good catch on backwards incompatibility. I think we will have to leave "null" as a possible type and null as an enum option, as I think some publishers do have compiledRelease containing null (cc @yolile in case you remember).

Similarly, we will have to leave 'compiled' as a possible code, since existing OCDS data does use that tag. (OCDS Merge uses it.)

In the time since this issue was created, we basically solved it by changing the descriptions of the problematic fields.

  • id

    … For a compiled release, the release ID should be the ocid and the maximum date among the individual releases used to create the compiled release, separated by a hyphen: {ocid}-{date}.

  • date

    … For a compiled release, the maximum date among the individual releases used to create the compiled release. …

As such, it looks like we don't need a new schema, and given the backwards-compatibility constraints, the new schema would basically be identical anyway.

What's left to do:

I think this means we leave omitWhenMerged, because we are indeed "omitting" the original value, and instead apply field-specific rules to re-populate those fields, following instructions in the field's description (and in the merging docs).

I was debating whether to deprecate the 'compiled' code. As-is, that code is used to determine whether a release is a compiled release. In principle, the rule is that a compiled release is anything that appears under compiledRelease in a record, and anything outside that is not a compiled release. In practice, we've seen release packages of compiled releases, etc. so I figure let's keep it as it serves as a useful marker when looking at JSON data in isolation.

@jpmckinney jpmckinney changed the title Make a compiledRelease schema Update release schema to be coherent with merging specification Oct 31, 2024
@odscjen
Copy link
Contributor

odscjen commented Nov 4, 2024

@jpmckinney Could you clarify these two points as they seem to be contradictory?

As such, it looks like we don't need a new schema, and given the backwards-compatibility constraints, the new schema would basically be identical anyway.

vs

Add a note to #1480 about creating a compiled-release-schema.json and removing "null" types and null enums from all fields.

@odscjen
Copy link
Contributor

odscjen commented Nov 4, 2024

We want to accumulate tag values per #330 (comment). This behavior is new – no other field behaves this way. So, we need to update the field description to describe this behavior.

Does this mean we want to replace "omitWhenMerged" with "wholeListMerge"? Or I am right in thinking this wouldn't work as what we're wanting is to merge the whole list but also add an additional tag? So leave it as is and just explain what's needed in the merging docs and tag schema description?

@jpmckinney
Copy link
Member

jpmckinney commented Nov 4, 2024

@jpmckinney Could you clarify these two points as they seem to be contradictory?

As such, it looks like we don't need a new schema, and given the backwards-compatibility constraints, the new schema would basically be identical anyway.

vs

Add a note to #1480 about creating a compiled-release-schema.json and removing "null" types and null enums from all fields.

The strict schema allows us to "break" backwards compatibility, because conformance to the strict schema is optional. For the strict schema, we can have a separate compiled release schema that has more restrictions than the release schema.

We want to accumulate tag values per #330 (comment). This behavior is new – no other field behaves this way. So, we need to update the field description to describe this behavior.

Does this mean we want to replace "omitWhenMerged" with "wholeListMerge"? Or I am right in thinking this wouldn't work as what we're wanting is to merge the whole list but also add an additional tag? So leave it as is and just explain what's needed in the merging docs and tag schema description?

No, wholeListMerge means "make a copy of the list from the most recent release of this contracting process" (which might be only "implementation" if that release only describes and changes the implementation). Tag's new semantics are "accumulate all unique values from all releases for this contracting process". We will not be adding a keyword to JSON Schema about this – we will just describe the rule in docs and field description. omitWhenMerged is still correct, because we do want to ignore the usual rules for merging.

@odscjen odscjen moved this from In progress to Review in progress in OCDS 1.2 Nov 5, 2024
@odscjen
Copy link
Contributor

odscjen commented Nov 5, 2024

It's also worth noting that whilst the merging rules guidance states that the id and date fields should be omitted from a compiled release, the example data on the same page of the docs shows a compiled release with both of these fields included.

The example data in https://standard.open-contracting.org/staging/1.2-dev/en/schema/merging/#merging-specification only includes id and date where it's showing the examples of the individual releases that are then merged into a versioned release so this isn't something that needs fixed. I'm assuming a previous update removed the referenced problem example data as there is no example of a compiled release in the docs now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Focus - Merging Relating to the creating of compiled releases and versioned releases Schema Relating to other changes in the JSON Schema (renamed fields, schema properties, etc.)
Projects
Status: Review in progress
Development

Successfully merging a pull request may close this issue.

8 participants