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

feat: Add SubjectResource entity and db migration #1048

Merged
merged 27 commits into from
Sep 4, 2024

Conversation

elsand
Copy link
Collaborator

@elsand elsand commented Aug 24, 2024

Description

This is the precursor to #875, adding the SubjectResource (was "RoleResource") entity and update mechanism that uses the new changefeed API in RR

This adds a "Janitor" console project, which can be invoked in container app jobs (or manually) to perform the syncronization.

Related Issue(s)

Verification

  • Your code builds clean without any errors or warnings
  • Manual testing done (required)
  • Relevant automated test added (if you find this hard, leave it and we'll help out)

Documentation

  • Documentation is updated (either in docs-directory, Altinnpedia or a separate linked PR in altinn-studio-docs., if applicable)

@elsand elsand changed the title Add SubjectResource entity and db migration feat: Add SubjectResource entity and db migration Aug 24, 2024
elsand and others added 3 commits August 30, 2024 17:34
<!--- Provide a general summary of your changes in the Title above -->

## Description
Adds:
- Dockerfile 
- Entry in a new docker-compose for jobs
- Bicep definitions and extra properties for specifying schedule and
command
- Github action to deploy jobs

<!--- Describe your changes in detail -->

## Related Issue(s)

- #{issue number}

## Verification

- [ ] **Your** code builds clean without any errors or warnings
- [ ] Manual testing done (required)
- [ ] Relevant automated test added (if you find this hard, leave it and
we'll help out)

## Documentation

- [ ] Documentation is updated (either in `docs`-directory, Altinnpedia
or a separate linked PR in
[altinn-studio-docs.](https://github.com/Altinn/altinn-studio-docs), if
applicable)
arealmaas and others added 6 commits September 3, 2024 10:38
Hvis dette godkjennes, fungerer det slik:
1. Hent sist oppdatert dato fra enten kommando input, eller den største
UpdatedAt i SubjectResources tabellen, eller DateTimeOffset.MinValue (i
den rekkefølgen).
2. Stream subjekt-ressurs-mappings batchvis fra ressursregisteret
3. Stapp det inn i DB via en merge statement
4. Repiter steg 2 og 3 til ressursregisteret ikke har noen flere
mappings

MERK - nå brukes ikke SubjectResourceLastUpdates tabellen ettersom siste
oppdateringsdato hentes fra SubjectResources. Jeg tror dette blir
korrekt. I tillegg gjør dette at vi ikke trenger å ta hensyn til server
clock drift ettersom SubjectResources.UpdatedAt hentes og populeres
direkte fra ressurs registeret. Altså vi bruker ikke DP sin tid, vi
bruker ressurs registeret sin tid. Eneste er at når vi er "ferdig" med
syncen ser jeg at ressurs register apiet fremdeles returnerer data, som
sier til meg at de henter data fra OG MED "since". IMO mener jeg dette
er feil. Det burde IMO kun vært fra, med et unntakt på likhet innenfor
tie-breakeren. Dette er den samme problemstillingen vi har løst på vår
paginering.

Jeg har tatt bort transaksnonshåndteringen fordi vi nå bruker en sql
merge statement for hele batchen. Dvs hver batch kan håndteres i hver
sin transaksjon. Men her kom jeg på noe. Dersom det såpass mange
subjekt-ressurs-mappinger innenfor samme timestamp at det strekker seg
over flere batcher og det skjer en exception kan man havne i en litt
uggen tilstand. Si for enkelhets skyld at det er 100 stk innenfor samme
tidspunkt og vi batcher i sett på 10. Dvs at vi må ha 10x10 successfull
batches for å traversere til et nytt tidspunkt. Men dersom vi failer på
batch nr 5 vil vi med steg 1 ovenfor hente ut det samme tidspunktet for
alle 100 stk og starte fra starten igjen (batch 1). Det er ikke et
problem slik apiet er nå (fra **og med** since), men dersom apiet endrer
seg til kun fra since blir det et issue. Vi kan løse det med å generere
vårt eget token som sendes inn til endepunktet for tie-breakeren, men da
må vi vite hvordan ressurs registeret sorterer responsen utover
UpdatedAt for å finne korrekt element i vår db for å lage token ut ifra.
Denne problemstillingen er ikke løst med SubjectResourceLastUpdates
tabellen heller, men det er kanskje ikke et stort problem.

Jeg mener dette er en forenkling av koden, og at det plasserer
ansvarsområdene på rett plass (infra huser sql queries, ikke app).
@elsand Bare huk tak i meg om du har noen spørsmål eller vil sparre om
løsningen 😁 Håper dette er til hjelp 🙂

PS - tok utgangspunkt i din gromme upsert metode når jeg lagde mergen! 🙌
MagnusSandgren
MagnusSandgren previously approved these changes Sep 4, 2024
MagnusSandgren
MagnusSandgren previously approved these changes Sep 4, 2024
oskogstad
oskogstad previously approved these changes Sep 4, 2024
@MagnusSandgren MagnusSandgren dismissed stale reviews from oskogstad and themself via 3841704 September 4, 2024 11:10
MagnusSandgren
MagnusSandgren previously approved these changes Sep 4, 2024
Co-authored-by: Ole Jørgen Skogstad <skogstad@softis.net>
@elsand elsand merged commit d04d764 into main Sep 4, 2024
12 checks passed
@elsand elsand deleted the feat/resourcesubject-mappings-sync branch September 4, 2024 13:36
Copy link

sonarcloud bot commented Sep 4, 2024

arealmaas pushed a commit that referenced this pull request Sep 10, 2024
🤖 I have created a release *beep* *boop*
---


##
[1.17.0](v1.16.0...v1.17.0)
(2024-09-10)


### Features

* Add SubjectResource entity and db migration
([#1048](#1048))
([d04d764](d04d764))
* **graphQL:** Add subscription for dialog details
([#1072](#1072))
([8214acb](8214acb))
* Implement scalable dialog search authorization
([#875](#875))
([aa8f84d](aa8f84d))
* revise dialog status
([#1099](#1099))
([0029f46](0029f46))


### Bug Fixes

* ensure correct appsettings is used
([#1086](#1086))
([d43f6d7](d43f6d7))
* ensure jobs are run with correct arguments and parameters
([#1085](#1085))
([e21de56](e21de56))
* **webapi:** Return 422 when existing transmission IDs are used in
dialog update
([#1094](#1094))
([7a8a933](7a8a933))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants