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

Jump to bottom

Adds endpoints for building search-context for single contentURI #173

Merged
merged 8 commits into from
Feb 1, 2023
Merged

Adds endpoints for building search-context for single contentURI #173

merged 8 commits into from
Feb 1, 2023

Conversation

jnatten
Copy link
Contributor

@jnatten jnatten commented Jan 25, 2023

Dette endepunktet vil forhåpentligvis ta over for oppbyggingen i search-api. I alle fall for enkelt-lagringer av artikler.
Vil forhåpentligvis gjøre lagring (indeksering av lagring) raskere og mer robust.

Vi kan vurdere om vi i fremtiden vil flytte all logikken for oppbyggingen av context til taxonomy-api for å slippe å ha duplisert logikk, men mistenker at det kanskje er litt jobb for å få det performant nok.

Kommer en PR i backend som kan brukes for å teste.

gunnarvelle
gunnarvelle reviewed Jan 26, 2023
View reviewed changes
Copy link
Member

@gunnarvelle gunnarvelle left a comment

Choose a reason for hiding this comment

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

CachedPath blei jo innført for å slippe å regne ut path til en node ved kvar henting, og det du gjør her er å gå tilbake til å gjøre akkurat det. I denne konteksten gir det meining i og med at du kun skal hente ut kontekster for indeksering. Men det hadde vore fint å kunne få kontekstinformasjon på andre endepunkt også.

Eg har prøvd å finne ut korleis vi skal kunne skille på kontekster for en ressurs som ligg under et fag og under et emne/morfag/programområde, og dette er eit svar på det. Men for å få til det treng vi å kunne hente kontekst basert på en node-id + rot-id. Altså at du ser på en ressurs under et programområde, og ved å sende inn id til programområdet til /v1/resources/?rootId= så får du pather relativt til rota og ikkje berre til subject.

Eg seier ikkje at vi skal implementere dette i denne pr, men det kan være lurt å ha det i bakhodet.

src/main/java/no/ndla/taxonomy/rest/v1/dtos/nodes/searchapi/SearchableTaxonomyContextDTO.java Outdated Show resolved Hide resolved
src/main/java/no/ndla/taxonomy/rest/v1/dtos/nodes/searchapi/SearchableTaxonomyContextDTO.java Outdated Show resolved Hide resolved
@jnatten
Add /queries/{contentUri} endpoint for building search contexts
d406fd5 
This endpoint will simplify building contexts for single articles. This
should speed up indexing a single article (or very few articles), fairly
dramatically as search-api no longer needs to fetch every node and their
connections.
@jnatten
Add tests for /v1/queries/{contentUri} endpoint
7fd4e72
jnatten and others added 3 commits January 31, 2023 06:15
@jnatten @gunnarvelle
Typo in doc
484b235 
Co-authored-by: Gunnar Velle <gunnar.velle@gmail.com>
@jnatten @gunnarvelle
Update src/main/java/no/ndla/taxonomy/rest/v1/dtos/nodes/searchapi/Se…
67b16bd 
…archableTaxonomyContextDTO.java

Co-authored-by: Gunnar Velle <gunnar.velle@gmail.com>
@jnatten
Fix topic parent ids to include all parent topics
b67a684
gunnarvelle
gunnarvelle approved these changes Jan 31, 2023
View reviewed changes
Copy link
Member

@gunnarvelle gunnarvelle left a comment

Choose a reason for hiding this comment

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

En liten forbedringsforslag. Ellers lgtm

src/main/java/no/ndla/taxonomy/rest/v1/dtos/nodes/searchapi/TaxonomyContextDTO.java

public record TaxonomyContextDTO(
@JsonProperty @Schema(description = "The public-id of the node connected via content-uri") URI id,
@JsonProperty @Schema(description = "The id of the root parent of the context") URI subjectId,
Copy link
Member

Choose a reason for hiding this comment

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

Kunne kanskje kalla denne rootId for å passe med dok.

Suggested change
@JsonProperty @Schema(description = "The id of the root parent of the context") URI subjectId,
@JsonProperty @Schema(description = "The id of the root parent of the context") URI rootId,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Den finnes allerede i denne, så hadde vært nice å bare beholdt de samme navnene enn så lenge.

Copy link
Member

Choose a reason for hiding this comment

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

Greit nok!

@jnatten jnatten mentioned this pull request Feb 1, 2023
Merged
@gunnarvelle
Copy link
Member

Kom på en ting. Tar denne koden høgde for at alle noder kan markeres som kontekst? Altså at et emne kan være kontekst og dermed ha gyldige adresser /subject/topic/topic og /topic/topic?

@jnatten jnatten merged commit a1f9096 into master Feb 1, 2023
@jnatten jnatten deleted the context-fetching-endpoint branch February 1, 2023 12:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gunnarvelle gunnarvelle gunnarvelle approved these changes

@haattis haattis Awaiting requested review from haattis

@Jonas-C Jonas-C Awaiting requested review from Jonas-C

@katrinewi katrinewi Awaiting requested review from katrinewi

@MaPoKen MaPoKen Awaiting requested review from MaPoKen

@N4G1 N4G1 Awaiting requested review from N4G1

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jnatten @gunnarvelle