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.

Sign up for GitHub

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

Spike: can we simplify the GraphQL types? #3024

Closed
wants to merge 1 commit into from
+112 −366
Closed

Spike: can we simplify the GraphQL types? #3024

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
184 changes: 111 additions & 73 deletions app/graphql/types/edition_type.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,121 @@ class WithdrawnNotice < Types::BaseObject
field :withdrawn_at, GraphQL::Types::ISO8601DateTime
end

class EditionLinks < Types::BaseObject
links_field :active_top_level_browse_page, [EditionType]
links_field :associated_taxons, [EditionType]
links_field :available_translations, [EditionType]
links_field :child_taxons, [EditionType]
links_field :children, [EditionType]
links_field :contact, [EditionType]
links_field :contacts, [EditionType]
links_field :content_owners, [EditionType]
links_field :corporate_information_pages, [EditionType]
links_field :current_prime_minister, [EditionType]
links_field :document_collections, [EditionType]
links_field :documents, [EditionType]
links_field :email_alert_signup, [EditionType]
links_field :embed, [EditionType]
links_field :fatality_notices, [EditionType]
links_field :featured_policies, [EditionType]
links_field :field_of_operation, [EditionType]
links_field :fields_of_operation, [EditionType]
links_field :finder, [EditionType]
links_field :government, [EditionType]
links_field :historical_accounts, [EditionType]
links_field :home_page_offices, [EditionType]
links_field :lead_organisations, [EditionType]
links_field :level_one_taxons, [EditionType]
links_field :linked_items, [EditionType]
links_field :main_office, [EditionType]
links_field :mainstream_browse_pages, [EditionType]
links_field :manual, [EditionType]
links_field :meets_user_needs, [EditionType]
links_field :ministerial, [EditionType]
links_field :ministers, [EditionType]
links_field :office_staff, [EditionType]
links_field :ordered_also_attends_cabinet, [EditionType]
links_field :ordered_assistant_whips, [EditionType]
links_field :ordered_baronesses_and_lords_in_waiting_whips, [EditionType]
links_field :ordered_board_members, [EditionType]
links_field :ordered_cabinet_ministers, [EditionType]
links_field :ordered_chief_professional_officers, [EditionType]
links_field :ordered_child_organisations, [EditionType]
links_field :ordered_contacts, [EditionType]
links_field :ordered_featured_policies, [EditionType]
links_field :ordered_foi_contacts, [EditionType]
links_field :ordered_high_profile_groups, [EditionType]
links_field :ordered_house_lords_whips, [EditionType]
links_field :ordered_house_of_commons_whips, [EditionType]
links_field :ordered_junior_lords_of_the_treasury_whips, [EditionType]
links_field :ordered_military_personnel, [EditionType]
links_field :ordered_ministerial_departments, [EditionType]
links_field :ordered_ministers, [EditionType]
links_field :ordered_parent_organisations, [EditionType]
links_field :ordered_related_items, [EditionType]
links_field :ordered_related_items_overrides, [EditionType]
links_field :ordered_roles, [EditionType]
links_field :ordered_special_representatives, [EditionType]
links_field :ordered_successor_organisations, [EditionType]
links_field :ordered_traffic_commissioners, [EditionType]
links_field :organisations, [EditionType]
links_field :original_primary_publishing_organisation, [EditionType]
links_field :pages_part_of_step_nav, [EditionType]
links_field :pages_related_to_step_nav, [EditionType]
links_field :pages_secondary_to_step_nav, [EditionType]
links_field :parent, [EditionType]
links_field :parent_taxons, [EditionType]
links_field :part_of_step_navs, [EditionType]
links_field :people, [EditionType]
links_field :person, [EditionType]
links_field :policies, [EditionType]
links_field :policy_areas, [EditionType]
links_field :popular_links, [EditionType]
links_field :primary_publishing_organisation, [EditionType]
links_field :primary_role_person, [EditionType]
links_field :related, [EditionType]
links_field :related_guides, [EditionType]
links_field :related_mainstream_content, [EditionType]
links_field :related_policies, [EditionType]
links_field :related_statistical_data_sets, [EditionType]
links_field :related_to_step_navs, [EditionType]
links_field :related_topics, [EditionType]
links_field :role, [EditionType]
links_field :role_appointments, [EditionType]
links_field :roles, [EditionType]
links_field :root_taxon, [EditionType]
links_field :second_level_browse_pages, [EditionType]
links_field :secondary_role_person, [EditionType]
links_field :secondary_to_step_navs, [EditionType]
links_field :sections, [EditionType]
links_field :service_manual_topics, [EditionType]
links_field :speaker, [EditionType]
links_field :sponsoring_organisations, [EditionType]
links_field :suggested_ordered_related_items, [EditionType]
links_field :take_part_pages, [EditionType]
links_field :taxonomy_topic_email_override, [EditionType]
links_field :taxons, [EditionType]
links_field :top_level_browse_pages, [EditionType]
links_field :topical_events, [EditionType]
links_field :world_locations, [EditionType]
links_field :worldwide_organisation, [EditionType]
links_field :worldwide_organisations, [EditionType]
end

field :active, Boolean, null: false
field :analytics_identifier, String
field :base_path, String
field :content_id, ID
field :current, Boolean
field :description, String
field :details, GraphQL::Types::JSON, null: false
field :document_type, String
field :ended_on, GraphQL::Types::ISO8601DateTime
field :first_published_at, GraphQL::Types::ISO8601DateTime, null: false
field :iso2, String
field :links, EditionLinks, method: :itself
field :locale, String, null: false
field :name, String, null: false
field :phase, String, null: false
field :public_updated_at, GraphQL::Types::ISO8601DateTime, null: false
field :publishing_app, String
Expand All @@ -23,9 +130,13 @@ class WithdrawnNotice < Types::BaseObject
field :rendering_app, String
field :scheduled_publishing_delay_seconds, Int
field :schema_name, String
field :slug, String, null: false
field :started_on, GraphQL::Types::ISO8601DateTime
field :state, String
field :supports_historical_accounts, Boolean
Copy link
Member

Choose a reason for hiding this comment

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

thought: approach

I'd be interested in discussing what our priorities are in terms of simplification vs clarity. Here are some questions off the top of my head (I haven't formed an opinion yet!):

  1. Here we might end up with one big edition type with fields that might or might not exist on a given document based on its type: will we then lose the ability to know whether a given document should have certain fields/links?
  2. Does the distinction between nullish data and invalid properties matter?
  3. Do we want GraphQL query clients to be able to make use of a complex schema for introspection, so that the clients can guide us on what could/should exist on a given document based on its type (this change would presumably mean they'd just suggest every field for every edition/link)?
  4. Do we want client apps to know via error if they're asking for data that will never exist on a given type (perhaps they're using it in a view and not realising it will always be null)?
  5. Is the benefit here mostly that we don't need to define fields on multiple types?
  6. Is this better than just moving the shared stuff into the edition type?
  7. Is everything technically shared, and just null on certain types?

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 everything technically shared, and just null on certain types?

This was my assumption when I started looking into this, on the basis that Publishing API can theoretically have any of these things on any document type in it's database, it's just we exclude them from being allowed by validating against a schema when the publishing application makes a request.

I don't know if it's possible (or even a good idea), but maybe we could validate GraphQL queries against the schemas too, so you can't request a field that isn't in that document's schema?

(Writing the answer to this question here, so I don't forgot before we mob)

field :title, String, null: false
field :updated_at, GraphQL::Types::ISO8601DateTime
field :web_url, String
field :withdrawn_notice, WithdrawnNotice

def withdrawn_notice
Expand All @@ -43,79 +154,6 @@ def not_stored_in_publishing_api
alias_method :publishing_scheduled_at, :not_stored_in_publishing_api
alias_method :scheduled_publishing_delay_seconds, :not_stored_in_publishing_api

class Translation < Types::BaseObject
field :locale, String
field :base_path, String
end

class GovernmentDetails < Types::BaseObject
field :current, Boolean
end

class GovernmentLink < Types::BaseObject
field :base_path, String
field :content_id, String
field :details, GovernmentDetails
field :title, String
end

class OrganisationLink < Types::BaseObject
field :base_path, String
field :content_id, String
field :title, String
end

class PersonLink < Types::BaseObject
field :base_path, String
field :content_id, String
field :title, String
end

class Taxon < Types::BaseObject
field :base_path, String
field :content_id, String
field :document_type, String
field :phase, String
field :title, String
end

class TaxonLink < Taxon
class TaxonLinks < Types::BaseObject
links_field :parent_taxons, [Taxon]
end

field :links, TaxonLinks, method: :itself
end

class TopicalEventLink < Types::BaseObject
field :base_path, String
field :content_id, String
field :title, String
end

class WorldLocationLink < Types::BaseObject
field :base_path, String
field :content_id, String
field :title, String
end

class EditionLinks < Types::BaseObject
field :available_translations, [Translation]
links_field :government, [GovernmentLink]
links_field :organisations, [OrganisationLink]
links_field :people, [PersonLink]
links_field :taxons, [TaxonLink]
links_field :topical_events, [TopicalEventLink]
links_field :world_locations, [WorldLocationLink]

def available_translations
Presenters::Queries::AvailableTranslations.by_edition(object)
.translations.fetch(:available_translations, [])
end
end

field :links, EditionLinks, method: :itself

private

def presented_edition
Expand Down
135 changes: 0 additions & 135 deletions app/graphql/types/ministers_index_type.rb
View file
Open in desktop

This file was deleted.

12 changes: 0 additions & 12 deletions app/graphql/types/news_article_type.rb
View file
Open in desktop

This file was deleted.

8 changes: 1 addition & 7 deletions app/graphql/types/query_type/edition_type_or_subtype.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,7 @@
module Types
class QueryType
class EditionTypeOrSubtype < Types::BaseUnion
EDITION_TYPES = [
Types::EditionType,
Types::MinistersIndexType,
Types::NewsArticleType,
Types::RoleType,
Types::WorldIndexType,
].freeze
EDITION_TYPES = [Types::EditionType].freeze

possible_types(*EDITION_TYPES)

Expand Down
Loading
Loading